[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r377340652 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -181,7 +181,11 @@ /** Configuration key for the list of master host:ports **/ public static final String MASTER_ADDRS_KEY = "hbase.masters"; - public static final String MASTER_ADDRS_DEFAULT = "localhost:" + DEFAULT_MASTER_PORT; Review comment: Ah okay. Let's avoid adding any fields to `HConstants` on the feature branch if we can. 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 With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r376624361 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java ## @@ -66,4 +68,24 @@ public static String getDefaultHost(String strInterface, String nameserver) return org.apache.hadoop.net.DNS.getDefaultHost(strInterface, nameserver); } } + + /** + * Get the configured hostname for master/regionserver. Gets the default hostname if not specified + * in the configuration. + * @param conf Configuration to look up. + * @param isMaster True if master hostname needs to be looked up and false for regionserver. + */ + public static String getHostname(Configuration conf, boolean isMaster) Review comment: Since this is now in a more public space, and since we have more service types than just "master" and "regionserver", perhaps the `boolean isMaster` should become `String serviceName`. 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 With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r376626826 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java ## @@ -66,4 +68,24 @@ public static String getDefaultHost(String strInterface, String nameserver) return org.apache.hadoop.net.DNS.getDefaultHost(strInterface, nameserver); } } + + /** + * Get the configured hostname for master/regionserver. Gets the default hostname if not specified + * in the configuration. + * @param conf Configuration to look up. + * @param isMaster True if master hostname needs to be looked up and false for regionserver. + */ + public static String getHostname(Configuration conf, boolean isMaster) Review comment: This method has a number of commits sprinkled into its recent history: HBASE-12954, HBASE-12956, HBASE-13481. They appear related to property binding to `0.0.0.0` and to a configuration with multiple IP's on a network interface. How do we test changes in this part of our code? 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 With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r376628904 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -181,7 +181,11 @@ /** Configuration key for the list of master host:ports **/ public static final String MASTER_ADDRS_KEY = "hbase.masters"; - public static final String MASTER_ADDRS_DEFAULT = "localhost:" + DEFAULT_MASTER_PORT; Review comment: Head's up, this file is `InterfaceAudience.Public`, which means removals have to go through a full release deprecation cycle. From the sub-section titled "Client API compatibility" in [the book](http://hbase.apache.org/book.html#hbase.versioning.post10): > An API needs to be deprecated for a whole major version before we will change/remove it. > * An example: An API was deprecated in 2.0.1 and will be marked for deletion in 4.0.0. On the other hand, an API deprecated in 2.0.0 can be removed in 3.0.0. You'll need to retain the hold field -- annotate it as deprecated and add a java doc explaining that the other method could be used in its place. You can file a follow-on ticket with a fixVersion of 4.0.0 as a task to delete the field once we get there (and maybe leave a TODO referencing that ticket). Sadly this constant is a field not a method, which means we have no way to logging a WARN for clients who make use of it. 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 With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r376631649 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -181,7 +181,11 @@ /** Configuration key for the list of master host:ports **/ public static final String MASTER_ADDRS_KEY = "hbase.masters"; - public static final String MASTER_ADDRS_DEFAULT = "localhost:" + DEFAULT_MASTER_PORT; + // key to the config parameter of server hostname + // the specification of server hostname is optional. The hostname should be resolvable from + // both master and region server + public static final String RS_HOSTNAME_KEY = "hbase.regionserver.hostname"; Review comment: Don't add these here. In [the book](http://hbase.apache.org/book.html#hbase_default_configurations), they're considered "expert" configuration params (and the `master` version appears to not be documented at all). Instead, add them to the `DNS` class, from which they'll be used. Also, it's `InterfaceAudience.Private` so we have more maintenance/refactoring flexibility there. And don't lose their `InterfaceAudience.LimitedPrivate` annotations when you move them. 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 With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry
ndimiduk commented on a change in pull request #1137: HBASE-23804: Fix default master addr hostname in master registry URL: https://github.com/apache/hbase/pull/1137#discussion_r375617103 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java ## @@ -20,6 +20,7 @@ import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK; import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS; import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK; +import static org.apache.hadoop.hbase.HConstants.MASTER_HOSTNAME_KEY; Review comment: Unused import? 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 With regards, Apache Git Services
