HBase
  1. HBase
  2. HBASE-1373

Update Thrift to use compact/framed protocol

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.90.0
    • Component/s: Thrift
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      TCompactProtocol/TFramedTransport and nonblocking server option promises better efficiency and performance improvements. Consider moving HBase Thrift bits to this when full platform support is ready for TCompactProtocol.

      1. javadoc.patch
        0.5 kB
        Lars Francke
      2. HBase-65+1373.patch
        22 kB
        Lars Francke
      3. HBase-65+1373.3.patch
        14 kB
        Lars Francke
      4. HBase-65+1373.2.patch
        14 kB
        Lars Francke

        Issue Links

          Activity

          Andrew Purtell created issue -
          Show
          Andrew Purtell added a comment - Some background: https://issues.apache.org/jira/browse/HBASE-1367?focusedCommentId=12706191&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12706191
          stack made changes -
          Field Original Value New Value
          Fix Version/s 0.21.0 [ 12313607 ]
          Hide
          Lars Francke added a comment -

          As far as I know this protocol is only implemented for C++, Ruby and Java so I wouldn't use it as a standard option (mostly because I need Python ) but as I am rewriting the Thrift interface anyway (HBASE-1744) I could add an option to choose between the current binary and the new compact protocol at startup. If needed two instances could be started on two different ports with different protocols. (Same goes for the FramedTransport which seems to be implemented for most languages)

          Would that be okay?

          Show
          Lars Francke added a comment - As far as I know this protocol is only implemented for C++, Ruby and Java so I wouldn't use it as a standard option (mostly because I need Python ) but as I am rewriting the Thrift interface anyway ( HBASE-1744 ) I could add an option to choose between the current binary and the new compact protocol at startup. If needed two instances could be started on two different ports with different protocols. (Same goes for the FramedTransport which seems to be implemented for most languages) Would that be okay?
          Hide
          ryan rawson added a comment -

          Lars is dead on here - compact/framed doesnt work for php, thus it's a non starter for us. Maybe for C++ or Ruby access, but Java, you might as well use the native client.

          Show
          ryan rawson added a comment - Lars is dead on here - compact/framed doesnt work for php, thus it's a non starter for us. Maybe for C++ or Ruby access, but Java, you might as well use the native client.
          Andrew Purtell made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Won't Fix [ 2 ]
          Hide
          Lars Francke added a comment -

          Could we get this reopened and possibly assigned to me?

          I'll use this to implement the compact protocol and framed transport as options for the old Thrift API.

          Show
          Lars Francke added a comment - Could we get this reopened and possibly assigned to me? I'll use this to implement the compact protocol and framed transport as options for the old Thrift API.
          Hide
          stack added a comment -

          Assigning LarsF at his request.

          Show
          stack added a comment - Assigning LarsF at his request.
          stack made changes -
          Resolution Won't Fix [ 2 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Assignee Lars Francke [ lars_francke ]
          Lars Francke made changes -
          Link This issue is blocked by HBASE-1360 [ HBASE-1360 ]
          Hide
          Lars Francke added a comment -

          HBASE-65 wasn't actually fixed with HBASE-1360, that must have been a misunderstanding. I'll do both here and I've got a patch ready that implements parts of both tickets.

          This patch adds various command line switches. It also solves HBASE-65 but it wasn't sensible to split those in two different patches.

          • Option to use a framed transport
          • Option to use the compact protocol
          • Option to bind to a specific IP address

          One change in behavior but I don't think anyone is affected by it and I'd call it a bugfix (although it makes the command line parsing ugly).

          bin/hbase should print usage information when invoked without an argument. The current Thrift server starts when no argument is provided: bin/hbase thrift. With this change the Thrift server prints usage information when called this way and only starts when the start argument is provided.

          Apart from this everything should stay the same as before as the defaults stayed the same.

          I've also added documentation for this but as english is my second language I'd appreciate a review.
          For the links in the documentation to actually work we need to generate Javadoc for the generated Thrift classes but those classes are currently excluded in the build.xml. I've added another patch that deals with this and another outdated exclusion (onelab was completely removed in September). I decided not to open another issue as it seems we might move to Maven anytime now but I can if you want.

          This also lacks the option for a nonblocking server. This is because the Nonblocking Servers require a TNonblockingServerSocket which doesn't support an InetSocketAddress. So the IP address binding only works for the normal TThreadPoolServer. I'll have a look at this and will open a ticket for Thrift.

          tl;dr: The patch fixes HBASE-65 + HBASE-1373 except that it only supports the old ThreadpoolServer and not the Nonblocking servers. This is for review only. I'll try to fix the rest.

          Show
          Lars Francke added a comment - HBASE-65 wasn't actually fixed with HBASE-1360 , that must have been a misunderstanding. I'll do both here and I've got a patch ready that implements parts of both tickets. This patch adds various command line switches. It also solves HBASE-65 but it wasn't sensible to split those in two different patches. Option to use a framed transport Option to use the compact protocol Option to bind to a specific IP address One change in behavior but I don't think anyone is affected by it and I'd call it a bugfix (although it makes the command line parsing ugly). bin/hbase should print usage information when invoked without an argument. The current Thrift server starts when no argument is provided: bin/hbase thrift . With this change the Thrift server prints usage information when called this way and only starts when the start argument is provided. Apart from this everything should stay the same as before as the defaults stayed the same. I've also added documentation for this but as english is my second language I'd appreciate a review. For the links in the documentation to actually work we need to generate Javadoc for the generated Thrift classes but those classes are currently excluded in the build.xml. I've added another patch that deals with this and another outdated exclusion (onelab was completely removed in September). I decided not to open another issue as it seems we might move to Maven anytime now but I can if you want. This also lacks the option for a nonblocking server. This is because the Nonblocking Servers require a TNonblockingServerSocket which doesn't support an InetSocketAddress. So the IP address binding only works for the normal TThreadPoolServer. I'll have a look at this and will open a ticket for Thrift. tl;dr: The patch fixes HBASE-65 + HBASE-1373 except that it only supports the old ThreadpoolServer and not the Nonblocking servers. This is for review only. I'll try to fix the rest.
          Lars Francke made changes -
          Attachment HBase-65+1373.patch [ 12430975 ]
          Attachment javadoc.patch [ 12430976 ]
          Hide
          stack added a comment -

          Pardon me Lars. I saw the --port arg. and read it as ip. I'll leave it closed for now since looks like you are about to fix it anyways.

          I think the above change an improvement.

          Let me add the javadoc.patch now.

          Show
          stack added a comment - Pardon me Lars. I saw the --port arg. and read it as ip. I'll leave it closed for now since looks like you are about to fix it anyways. I think the above change an improvement. Let me add the javadoc.patch now.
          Hide
          stack added a comment -

          I made hbase-2151 to add the javadoc but there are 82 warnings that need fixing before I can apply it.

          I took a look at this patch. There is nothing wrong with the english. Its great. The rest of the patch looks fine to me (Try and avoid white-space-only changes if you can making patches in future). I didn't test it though. Let me know when you think it ready and I will try it out.

          Show
          stack added a comment - I made hbase-2151 to add the javadoc but there are 82 warnings that need fixing before I can apply it. I took a look at this patch. There is nothing wrong with the english. Its great. The rest of the patch looks fine to me (Try and avoid white-space-only changes if you can making patches in future). I didn't test it though. Let me know when you think it ready and I will try it out.
          Hide
          Lars Francke added a comment -

          I've opened a ticket for this in Thrft (THRIFT-684) and it is already committed. I've fixed ThriftServer so that the --bind parameter can only be used on the ThreadPoolServer for now. We'll have to wait until Thrift 0.3 for the other to servers to support IP address binding. Really nothing else I can do at the moment. I'll attach an updated and tested patch tomorrow.

          Show
          Lars Francke added a comment - I've opened a ticket for this in Thrft ( THRIFT-684 ) and it is already committed. I've fixed ThriftServer so that the --bind parameter can only be used on the ThreadPoolServer for now. We'll have to wait until Thrift 0.3 for the other to servers to support IP address binding. Really nothing else I can do at the moment. I'll attach an updated and tested patch tomorrow.
          Hide
          Lars Francke added a comment -

          And I'm sorry for the extra whitespace. I'll make sure the next patch is clean.

          Show
          Lars Francke added a comment - And I'm sorry for the extra whitespace. I'll make sure the next patch is clean.
          Hide
          stack added a comment -

          Patch as I say looks good. No javadoc warnings. Let me know when ready to commit. We can open a new issue to cover Nonblocking servers?

          Show
          stack added a comment - Patch as I say looks good. No javadoc warnings. Let me know when ready to commit. We can open a new issue to cover Nonblocking servers?
          Hide
          Lars Francke added a comment -

          This should be the final patch. It includes the option to start the two Nonblocking servers. I think I've documented everything.

          I've also incorporated the changes that were required because of the Nonblocking servers not yet supporting IP address binding.

          Show
          Lars Francke added a comment - This should be the final patch. It includes the option to start the two Nonblocking servers. I think I've documented everything. I've also incorporated the changes that were required because of the Nonblocking servers not yet supporting IP address binding.
          Lars Francke made changes -
          Attachment HBase-65+1373.2.patch [ 12431082 ]
          Hide
          Lars Francke added a comment -

          Updated patch after HBASE-2153 has been applied (ThriftServer is the same, only change is to package.html)

          Show
          Lars Francke added a comment - Updated patch after HBASE-2153 has been applied (ThriftServer is the same, only change is to package.html)
          Lars Francke made changes -
          Attachment HBase-65+1373.3.patch [ 12431212 ]
          Hide
          stack added a comment -

          Committed to TRUNK. Thanks for the patch Lars Francke.

          Show
          stack added a comment - Committed to TRUNK. Thanks for the patch Lars Francke.
          stack made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]

            People

            • Assignee:
              Lars Francke
              Reporter:
              Andrew Purtell
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development