HBase
  1. HBase
  2. HBASE-4457

Starting region server on non-default info port is resulting in broken URL's in master UI

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Implemented
    • Affects Version/s: 0.92.0
    • Fix Version/s: None
    • Component/s: master
    • Labels:
    • Release Note:
      This patch is to remove the region server web port hard coding.

      Description

      When "hbase.regionserver.info.port" is set to non-default port, Master UI has broken URL's in the region server table because it's hard coded to default port.

      1. 4457-V1.patch
        8 kB
        Praveen Patibandla
      2. 4457.patch
        8 kB
        Praveen Patibandla

        Activity

        Hide
        Ted Yu added a comment -

        Reformatting is needed for patch v1:

        +      result = this.hbaseMaster.regionServerStartup(port, this.startcode, now,webport);
        

        There should be a space after the last comma.

        Tab should be two spaces.

        Overall, patch looks good.

        I will wait for Stack's comment.

        Show
        Ted Yu added a comment - Reformatting is needed for patch v1: + result = this .hbaseMaster.regionServerStartup(port, this .startcode, now,webport); There should be a space after the last comma. Tab should be two spaces. Overall, patch looks good. I will wait for Stack's comment.
        Hide
        Praveen Patibandla added a comment -

        @Ted, Thank you for reviewing the patch. I've fixed the formatting.

        Show
        Praveen Patibandla added a comment - @Ted, Thank you for reviewing the patch. I've fixed the formatting.
        Hide
        stack added a comment -

        @Praveen I thought about going this route. I couldn't convince myself that webport was important enough info to make it into the ServerName fundamental type (its used everywhere, in fs and persisted to zk, serialized over rpc) and truth be told, I punted on what to do about it; thanks for raising it again.

        So I still think having webui port in the servername is not the right place for it.

        What about adding webui param to HMasterRegionInterface#regionServerStartup?

        Pass it through to ServerManager.

        OnlineServers is ServerName->HServerLoad Map. Maybe the value needs to change to be a datastructure that has HSL and webui? That would be pretty disruptive? Perhaps a new ServerInfo or ServerVitals that has HSL and webui? Maybe later we'll add more 'server info' to this data structure?

        I'd almost be ok with a new map of SN->webui that sat beside the onlineServers and was expired when onlineServer expired........

        Sorry if this is more than you bargained for.

        Show
        stack added a comment - @Praveen I thought about going this route. I couldn't convince myself that webport was important enough info to make it into the ServerName fundamental type (its used everywhere, in fs and persisted to zk, serialized over rpc) and truth be told, I punted on what to do about it; thanks for raising it again. So I still think having webui port in the servername is not the right place for it. What about adding webui param to HMasterRegionInterface#regionServerStartup? Pass it through to ServerManager. OnlineServers is ServerName->HServerLoad Map. Maybe the value needs to change to be a datastructure that has HSL and webui? That would be pretty disruptive? Perhaps a new ServerInfo or ServerVitals that has HSL and webui? Maybe later we'll add more 'server info' to this data structure? I'd almost be ok with a new map of SN->webui that sat beside the onlineServers and was expired when onlineServer expired........ Sorry if this is more than you bargained for.
        Hide
        Todd Lipcon added a comment -

        I'd almost be ok with a new map of SN->webui that sat beside the onlineServers and was expired when onlineServer expired........

        sounds like lots of room for leaks/errors..

        I much prefer the idea that we collapse the existing maps to have a single class called something like RSOnMaster which is the info that the master tracks about each RS.

        Show
        Todd Lipcon added a comment - I'd almost be ok with a new map of SN->webui that sat beside the onlineServers and was expired when onlineServer expired........ sounds like lots of room for leaks/errors.. I much prefer the idea that we collapse the existing maps to have a single class called something like RSOnMaster which is the info that the master tracks about each RS.
        Hide
        Praveen Patibandla added a comment -

        Thank you Stack and Todd for your inputs. I will implement single data structure to capture server info and HSL . My only concern is HSL is dynamic but server info is static.

        Show
        Praveen Patibandla added a comment - Thank you Stack and Todd for your inputs. I will implement single data structure to capture server info and HSL . My only concern is HSL is dynamic but server info is static.
        Hide
        stack added a comment -

        @Praveen Yeah.. so I'd think there'd be a setHSL on the combined datastructure into which you could add latest HSL gotten from RS.

        Show
        stack added a comment - @Praveen Yeah.. so I'd think there'd be a setHSL on the combined datastructure into which you could add latest HSL gotten from RS.
        Hide
        Praveen Patibandla added a comment -

        Thanks Stack, I was initially thinking of replacing the HSL with new data structure.

        Show
        Praveen Patibandla added a comment - Thanks Stack, I was initially thinking of replacing the HSL with new data structure.
        Hide
        Jean-Daniel Cryans added a comment -

        A temporary fix could be done by using the configuration directly instead of asking the region server. Most of the time the master and RSs share that same file so it should cover a lot more cases than the current code does.

        Show
        Jean-Daniel Cryans added a comment - A temporary fix could be done by using the configuration directly instead of asking the region server. Most of the time the master and RSs share that same file so it should cover a lot more cases than the current code does.
        Hide
        stack added a comment -

        Temporary fix sounds good J-D.

        And after talking more w/ J-D, one scenario we don't handle w/ the above suggested passing of webuiport on regionserver start is case where all regionservers are already floating when the master comes up. In this situation, there will be no start message on which the regionserver volunteers UI port – regionservers are just heartbeating. Unless we made regionservers rerun their start message – which we might want to do someday though, could be issues if the new master tried passing config that was different from what the regionserver stared with – or we have master persist regionserver stats to zk (or when regionserver registers in zk, in its payload it puts its vitals such as webuiport... which we might want to do) a new master rising into an already-running cluster would be a little hosed.

        Show
        stack added a comment - Temporary fix sounds good J-D. And after talking more w/ J-D, one scenario we don't handle w/ the above suggested passing of webuiport on regionserver start is case where all regionservers are already floating when the master comes up. In this situation, there will be no start message on which the regionserver volunteers UI port – regionservers are just heartbeating. Unless we made regionservers rerun their start message – which we might want to do someday though, could be issues if the new master tried passing config that was different from what the regionserver stared with – or we have master persist regionserver stats to zk (or when regionserver registers in zk, in its payload it puts its vitals such as webuiport... which we might want to do) a new master rising into an already-running cluster would be a little hosed.
        Hide
        Todd Lipcon added a comment -

        Exposing the web UI port in ZK seems like the right fix to me. But the temporary fix makes sense in the meantime.

        Show
        Todd Lipcon added a comment - Exposing the web UI port in ZK seems like the right fix to me. But the temporary fix makes sense in the meantime.
        Hide
        stack added a comment -

        Making this critical; its an ugly regression not being able to browse to regionserver from master UI

        Show
        stack added a comment - Making this critical; its an ugly regression not being able to browse to regionserver from master UI
        Hide
        Praveen Patibandla added a comment -

        Temporary fix won't work if we start multiple region servers on the same machine.I'm not able to allocate my time to work on this item.I hope someone else can pick it up.

        Show
        Praveen Patibandla added a comment - Temporary fix won't work if we start multiple region servers on the same machine.I'm not able to allocate my time to work on this item.I hope someone else can pick it up.
        Hide
        stack added a comment -

        @Praveen No problem. Yeah the temporary workaround won't work if multiple regionservers on single machine. Thats rare though I'd say.

        Show
        stack added a comment - @Praveen No problem. Yeah the temporary workaround won't work if multiple regionservers on single machine. Thats rare though I'd say.
        Hide
        stack added a comment -

        Marking this down to major since it looks like we do read from the configuration in current trunk and branch:

        ...
        200 <table>
        201 <tr><th rowspan="<% servers.size() + 1%>"></th><th>ServerName</th><th>Load</th></tr>
        202 <%java>
        203    ServerName [] serverNames = servers.toArray(new ServerName[servers.size()]);
        204      Arrays.sort(serverNames);
        205      for (ServerName serverName: serverNames) {
        206        // TODO: this is incorrect since this conf might differ from RS to RS
        207        // or be set to 0 to get ephemeral ports
        208        int infoPort = master.getConfiguration().getInt("hbase.regionserver.info.port", 60030);
        209        String url = "http://" + serverName.getHostname() + ":" + infoPort + "/";
        210        HServerLoad hsl = master.getServerManager().getLoad(serverName);
        211        String loadStr = hsl == null? "-": hsl.toString();
        212        if (hsl != null) {
        213          totalRegions += hsl.getNumberOfRegions();
        214          totalRequests += hsl.getNumberOfRequests();
        215        }
        216 </%java>
        
        Show
        stack added a comment - Marking this down to major since it looks like we do read from the configuration in current trunk and branch: ... 200 <table> 201 <tr><th rowspan= "<% servers.size() + 1%>" ></th><th>ServerName</th><th>Load</th></tr> 202 <%java> 203 ServerName [] serverNames = servers.toArray( new ServerName[servers.size()]); 204 Arrays.sort(serverNames); 205 for (ServerName serverName: serverNames) { 206 // TODO: this is incorrect since this conf might differ from RS to RS 207 // or be set to 0 to get ephemeral ports 208 int infoPort = master.getConfiguration().getInt( "hbase.regionserver.info.port" , 60030); 209 String url = "http: //" + serverName.getHostname() + ":" + infoPort + "/" ; 210 HServerLoad hsl = master.getServerManager().getLoad(serverName); 211 String loadStr = hsl == null ? "-" : hsl.toString(); 212 if (hsl != null ) { 213 totalRegions += hsl.getNumberOfRegions(); 214 totalRequests += hsl.getNumberOfRequests(); 215 } 216 </%java>
        Hide
        Lars Hofhansl added a comment -

        Moving to 0.94.1 for now.

        Show
        Lars Hofhansl added a comment - Moving to 0.94.1 for now.
        Hide
        stack added a comment -

        Done recently by Liu Shaohui

        Show
        stack added a comment - Done recently by Liu Shaohui

          People

          • Assignee:
            Unassigned
            Reporter:
            Praveen Patibandla
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development