[ https://issues.apache.org/jira/browse/HBASE-5083?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13692245#comment-13692245 ]
Jesse Yates edited comment on HBASE-5083 at 6/24/13 6:51 PM: ------------------------------------------------------------- Looking at the trunk patch, a couple of nits: in hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/BackupMasterStatusTmpl.jamon {quote} +Copyright 2011 The Apache Software Foundation {quote} isn't needed. {quote} + </tr> + <%java> + Arrays.sort(serverNames); + for (ServerName serverName: serverNames) { + </%java> {quote} Spacing looks a little bit off. In HMaster, {quote} + if(master.isActiveMaster()){ + metaLocation = getMetaLocationOrNull(master); + //ServerName metaLocation = master.getCatalogTracker().getMetaLocation(); + servers = master.getServerManager().getOnlineServersList(); + deadServers = master.getServerManager().getDeadServers().copyServerNames(); + } {quote} Spacing is off (everything else looks to be 2 spaces, not 4 (or is that a tab? can't tell just reading the text diff)). {quote} + return (master.getCatalogTracker() == null) ? null : master.getCatalogTracker().getMetaLocation(); {quote} Wish there wasn't a need for the null here and instead a special ServerName that we could use when its null (increases potential for NullPointerExceptions, makes code a little more brittle, requires more null checks other places, etc.), but just complaining - this is fine and fits in with everything else. otherwise, it looks fine. +1, if you wouldn't mind fixing the nits on commit. was (Author: jesse_yates): nits: in hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/BackupMasterStatusTmpl.jamon {quote} +Copyright 2011 The Apache Software Foundation {quote} isn't needed. {quote} + </tr> + <%java> + Arrays.sort(serverNames); + for (ServerName serverName: serverNames) { + </%java> {quote} Spacing looks a little bit off. In HMaster, {quote} + if(master.isActiveMaster()){ + metaLocation = getMetaLocationOrNull(master); + //ServerName metaLocation = master.getCatalogTracker().getMetaLocation(); + servers = master.getServerManager().getOnlineServersList(); + deadServers = master.getServerManager().getDeadServers().copyServerNames(); + } {quote} Spacing is off (everything else looks to be 2 spaces, not 4 (or is that a tab? can't tell just reading the text diff)). {quote} + return (master.getCatalogTracker() == null) ? null : master.getCatalogTracker().getMetaLocation(); {quote} Wish there wasn't a need for the null here and instead a special ServerName that we could use when its null (increases potential for NullPointerExceptions, makes code a little more brittle, requires more null checks other places, etc.), but just complaining - this is fine and fits in with everything else. otherwise, it looks fine. +1, if you wouldn't mind fixing the nits on commit. > Backup HMaster should have http infoport open with link to the active master > ---------------------------------------------------------------------------- > > Key: HBASE-5083 > URL: https://issues.apache.org/jira/browse/HBASE-5083 > Project: HBase > Issue Type: Improvement > Components: master > Affects Versions: 0.92.0 > Reporter: Jonathan Hsieh > Assignee: Cody Marcel > Fix For: 0.94.9 > > Attachments: backup_master.png, HBASE-5083.patch, HBASE-5083.patch, > HBASE-5083.patch, HBASE-5083.patch, HBASE-5083.patch, HBASE-5083_trunk.patch, > HBASE-5083_trunk.patch, HBASE-5083_trunk.patch, master.png, > Trunk_Backup_Master.png, Trunk_Master.png > > > Without ssh'ing and jps/ps'ing, it is difficult to see if a backup hmaster is > up. It seems like it would be good for a backup hmaster to have a basic web > page up on the info port so that users could see that it is up. Also it > should probably either provide a link to the active master or automatically > forward to the active master. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira