HBase
  1. HBase
  2. HBASE-1744

Thrift server to match the new java api.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: Thrift
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      thrift, client

      Description

      This mutateRows, etc.. is a little confusing compared to the new cleaner java client.
      Thinking of ways to make a thrift client that is just as elegant. something like:

      void put(1:Bytes table, 2:TPut put) throws (1:IOError io)

      with:

      struct TColumn

      { 1:Bytes family, 2:Bytes qualifier, 3:i64 timestamp }

      struct TPut

      { 1:Bytes row, 2:map<TColumn, Bytes> values }

      This creates more verbose rpc than if the columns in TPut were just map<Bytes, map<Bytes, Bytes>>, but that is harder to fit timestamps into and still be intuitive from say python.

      Presumably the goal of a thrift gateway is to be easy first.

      1. 1744.addendum
        0.8 kB
        Ted Yu
      2. HBASE-1744.11.patch
        691 kB
        Tim Sell
      3. 1744-trunk.10
        691 kB
        Ted Yu
      4. HBASE-1744.9.patch
        691 kB
        Tim Sell
      5. HBASE-1744.8.patch
        687 kB
        Bob Copeland
      6. 0001-thrift2-enable-usage-of-.deleteColumns-for-thrift.patch
        14 kB
        Bob Copeland
      7. HBASE-1744.7.patch
        682 kB
        Tim Sell
      8. HBASE-1744.6.patch
        677 kB
        Tim Sell
      9. HBASE-1744.5.patch
        684 kB
        Tim Sell
      10. HBASE-1744.4.patch
        783 kB
        Tim Sell
      11. HBASE-1744.3.patch
        787 kB
        Tim Sell
      12. HBASE-1744.2.patch
        754 kB
        Lars Francke
      13. HBASE-1744.preview.1.patch
        64 kB
        Lars Francke
      14. thriftexperiment.patch
        14 kB
        Tim Sell

        Issue Links

          Activity

          Hide
          stack added a comment -

          I added this to the 0.21 roadmap.

          Show
          stack added a comment - I added this to the 0.21 roadmap.
          Hide
          stack added a comment -

          Ok to do HBASE-1402 as part of this issue? (Move thrift bindings out to contrib?)

          Show
          stack added a comment - Ok to do HBASE-1402 as part of this issue? (Move thrift bindings out to contrib?)
          Hide
          Tim Sell added a comment -

          +1

          Show
          Tim Sell added a comment - +1
          Hide
          Jonathan Gray added a comment -

          Note: Changes to thrift in trunk introduced by HBASE-1822

          Show
          Jonathan Gray added a comment - Note: Changes to thrift in trunk introduced by HBASE-1822
          Hide
          Jonathan Gray added a comment -

          Move to contrib per linked issues.

          Show
          Jonathan Gray added a comment - Move to contrib per linked issues.
          Hide
          Tim Sell added a comment -

          Sadly we aren't using HBase anymore and it's not looking like we'll ever use it in production. Though I'm still interested in this issue.
          I've barely looked at this in months so if someone else is interested it might be better they take it over.
          At the least I'd appreciate comments from anyone who uses the thrift server.

          Attached is what I was thinking for puts, gets and deletes.
          I'll add a few iterations over the next week.

          Show
          Tim Sell added a comment - Sadly we aren't using HBase anymore and it's not looking like we'll ever use it in production. Though I'm still interested in this issue. I've barely looked at this in months so if someone else is interested it might be better they take it over. At the least I'd appreciate comments from anyone who uses the thrift server. Attached is what I was thinking for puts, gets and deletes. I'll add a few iterations over the next week.
          Hide
          Tim Sell added a comment -

          Got an email from Lars Francke, who's doing some work on this.
          Feel free to take it over

          Show
          Tim Sell added a comment - Got an email from Lars Francke, who's doing some work on this. Feel free to take it over
          Hide
          Lars Francke added a comment -

          I'm currently working on this based on your latest patch. At the same time I'm moving it to contrib (HBASE-1846)

          What is the deprecation policy for this? Should I maintain the current API (it has to be regenerated for a newer version of Thrift) and introduce the new API as an additional option?
          As far as I know there is no option to mark something as deprecated in the thrift file so the generated code would not mention any deprecation. We'd have to document/communicate this.

          Show
          Lars Francke added a comment - I'm currently working on this based on your latest patch. At the same time I'm moving it to contrib ( HBASE-1846 ) What is the deprecation policy for this? Should I maintain the current API (it has to be regenerated for a newer version of Thrift) and introduce the new API as an additional option? As far as I know there is no option to mark something as deprecated in the thrift file so the generated code would not mention any deprecation. We'd have to document/communicate this.
          Hide
          Tim Sell added a comment -

          I wouldn't worry about breaking the current thrift api completely if it's a big improvement, contrib is always optional. It would be trivial for people continue using the old one if they wanted.

          Show
          Tim Sell added a comment - I wouldn't worry about breaking the current thrift api completely if it's a big improvement, contrib is always optional. It would be trivial for people continue using the old one if they wanted.
          Hide
          ryan rawson added a comment -

          It is a big deal for us to keep the current thrift API, we'd have to have a flag day and change our clients and servers at the same time, not ideal.

          It would be best if we could have both APIs available (that is how we did the 0.19 -> 0.20 (ish) migration).

          Show
          ryan rawson added a comment - It is a big deal for us to keep the current thrift API, we'd have to have a flag day and change our clients and servers at the same time, not ideal. It would be best if we could have both APIs available (that is how we did the 0.19 -> 0.20 (ish) migration).
          Hide
          Andrew Purtell added a comment -

          The way I would like to see this happen is for us to build a replacement Thrift connector to contrib/ (HBASE-1846) while keeping the current Thrift package in core for one release. This is what happened with Stargate. It and the old REST connector coexist in 0.20.x, with Stargate residing in contrib/src/stargate/, but only Stargate will remain in 0.21.x. This gives the freedom to do things differently (but, presumed, better) in the new contrib. So in the case of Thrift, there would be two connectors in 0.21, with the in-core connector scheduled to go away in 0.22.

          Show
          Andrew Purtell added a comment - The way I would like to see this happen is for us to build a replacement Thrift connector to contrib/ ( HBASE-1846 ) while keeping the current Thrift package in core for one release. This is what happened with Stargate. It and the old REST connector coexist in 0.20.x, with Stargate residing in contrib/src/stargate/, but only Stargate will remain in 0.21.x. This gives the freedom to do things differently (but, presumed, better) in the new contrib. So in the case of Thrift, there would be two connectors in 0.21, with the in-core connector scheduled to go away in 0.22.
          Hide
          stack added a comment - - edited

          Yeah, what Andrew says. We would deprecate current thrift API in 0.21 The new API would exist at same time but under contrib. as distinct software. The new API does not have to support or relate to the old (IMO). If a do-over from scratch makes sense, go for it (IMO). I would suggest modeling the java API if that will work Good stuff

          Show
          stack added a comment - - edited Yeah, what Andrew says. We would deprecate current thrift API in 0.21 The new API would exist at same time but under contrib. as distinct software. The new API does not have to support or relate to the old (IMO). If a do-over from scratch makes sense, go for it (IMO). I would suggest modeling the java API if that will work Good stuff
          Hide
          Lars Francke added a comment -

          Thanks for the input! Keeping the old thrift API around for now won't be a problem. I just needed to know how to proceed.

          I'll have an updated Hbase.thrift file ready in the next few days I hope and a patch shortly afterwards

          Show
          Lars Francke added a comment - Thanks for the input! Keeping the old thrift API around for now won't be a problem. I just needed to know how to proceed. I'll have an updated Hbase.thrift file ready in the next few days I hope and a patch shortly afterwards
          Hide
          Lars Francke added a comment -

          This is my first stab at a completely new (again, based on Tim's experimental idea) Thrift API. Quite a few points came up and I'd be interested in feedback:

          • So far I have not included Filters in any way as there is no struct subclassing in Thrift. So I'd need to come up with one data structure for all Filters. I thought of using a list with struct entries that contain a FilterType (enum) and a list or map of string options that would have to be parsed. Probably not the best solution so if anyone has an idea go ahead
          • A few things that are currently missing that I plan to add: Row lock, Scanner caching, maxVersions, incrementColumnValue and checkAndPut
          • A few things that I'm not yet sure about: The async version of createTable, closeRegion (both from HBaseAdmin), getServerInfo from ClusterStatus, quite a few methods from HTable that are not in HTableInterface (getRegionLocation, getStartKeys, getEndKeys, getRegionsInfo, getRegionLocation
          • What to do with autoFlush? I could easily add a method disableAutoflush or something like that but what if flushCommit is never called? How does HBase behave? I'd have to return a identifier for the table so that it can be reused in subsequent calls (just like the scanner interface)
          • I changed scanners to return longs before realizing that scanners timeout after 60 seconds (default) so I'll probably change it back to ints
          • Currently scanners are not cleaned up, I plan on adding a cleanup thread that automatically closes/removes old scanners from the list
          • Results are returned in TResult objects. You currently have to check if isSetRow or if values.size == 0 - I am thinking of adding an boolen field "empty" as this should be more reliable but as TResults will be sent quite often every this would probably cause a noticeable overhead (I'm not sure how well an optional field is handled in Thrift)
          • Some structs have the "T" as a prefix to avoid name clashes with HBase (Get -> TGet) but some don't (HTableDescriptor -> TableDescriptor, HColumnDescriptor -> ColumnDescriptor), should I use the "T" everywhere to be consistent?
          • The "delete" method had to be called something else in Thrift as "delete" is a reserved keyword. Tim used "delet" but I think that's quite confusing (I thought it was a type )so I opted for "deleteSingle" instead but I'm open for suggestions

          Well as I said: My first stab at this - I'm not very familiar with HBase so I hope I got most of it right. If there are no major concerns I'll go ahead and implement the rest of it (parts are done already) and upload a first patch.

          Thanks for all the help on IRC (larsgeorge, dj_ryan, jdcryans and the others)!

          Show
          Lars Francke added a comment - This is my first stab at a completely new (again, based on Tim's experimental idea) Thrift API. Quite a few points came up and I'd be interested in feedback: So far I have not included Filters in any way as there is no struct subclassing in Thrift. So I'd need to come up with one data structure for all Filters. I thought of using a list with struct entries that contain a FilterType (enum) and a list or map of string options that would have to be parsed. Probably not the best solution so if anyone has an idea go ahead A few things that are currently missing that I plan to add: Row lock, Scanner caching, maxVersions, incrementColumnValue and checkAndPut A few things that I'm not yet sure about: The async version of createTable, closeRegion (both from HBaseAdmin), getServerInfo from ClusterStatus, quite a few methods from HTable that are not in HTableInterface (getRegionLocation, getStartKeys, getEndKeys, getRegionsInfo, getRegionLocation What to do with autoFlush? I could easily add a method disableAutoflush or something like that but what if flushCommit is never called? How does HBase behave? I'd have to return a identifier for the table so that it can be reused in subsequent calls (just like the scanner interface) I changed scanners to return longs before realizing that scanners timeout after 60 seconds (default) so I'll probably change it back to ints Currently scanners are not cleaned up, I plan on adding a cleanup thread that automatically closes/removes old scanners from the list Results are returned in TResult objects. You currently have to check if isSetRow or if values.size == 0 - I am thinking of adding an boolen field "empty" as this should be more reliable but as TResults will be sent quite often every this would probably cause a noticeable overhead (I'm not sure how well an optional field is handled in Thrift) Some structs have the "T" as a prefix to avoid name clashes with HBase (Get -> TGet) but some don't (HTableDescriptor -> TableDescriptor, HColumnDescriptor -> ColumnDescriptor), should I use the "T" everywhere to be consistent? The "delete" method had to be called something else in Thrift as "delete" is a reserved keyword. Tim used "delet" but I think that's quite confusing (I thought it was a type )so I opted for "deleteSingle" instead but I'm open for suggestions Well as I said: My first stab at this - I'm not very familiar with HBase so I hope I got most of it right. If there are no major concerns I'll go ahead and implement the rest of it (parts are done already) and upload a first patch. Thanks for all the help on IRC (larsgeorge, dj_ryan, jdcryans and the others)!
          Hide
          stack added a comment -

          What if you just did one or two of the filters for now? If someone wants other filters on thrift-side bad enough, they can add them?

          IMO, leave the admin functions out of your thrift machinations; e.g. closeRegion, getServerInfo, getStartKeys, etc. Most of this utility can be done via shell. If really wanted, could do in another issue?

          The one exception to above rule would be revealing methods that might be used monitoring an hbase cluster; e.g. perhaps someone wants to write a perl script that checks cluster status and run it from cron.

          Do you not have a close on the scanner?

          OK on empty flag.

          On 'T', yeah, should be consistent I'd say.

          deleteSingle is fine by me.

          Looking at hbase.thrift....

          Stating the obvious, the package documentation is where you make your thrift explaination.

          Purge 'Text'. It doesn't seem to be used anyways and we went out of our way before you were born – way back in 0.18/0.19 purging hadoop Text from hbase.

          So, TResult is a map of the tuple family/qualifier/ts to value? That should work.

          Excellent Lars.

          Show
          stack added a comment - What if you just did one or two of the filters for now? If someone wants other filters on thrift-side bad enough, they can add them? IMO, leave the admin functions out of your thrift machinations; e.g. closeRegion, getServerInfo, getStartKeys, etc. Most of this utility can be done via shell. If really wanted, could do in another issue? The one exception to above rule would be revealing methods that might be used monitoring an hbase cluster; e.g. perhaps someone wants to write a perl script that checks cluster status and run it from cron. Do you not have a close on the scanner? OK on empty flag. On 'T', yeah, should be consistent I'd say. deleteSingle is fine by me. Looking at hbase.thrift.... Stating the obvious, the package documentation is where you make your thrift explaination. Purge 'Text'. It doesn't seem to be used anyways and we went out of our way before you were born – way back in 0.18/0.19 purging hadoop Text from hbase. So, TResult is a map of the tuple family/qualifier/ts to value? That should work. Excellent Lars.
          Hide
          Lars Francke added a comment -

          Thanks for the comments!

          I will have a look at the filters and see which can be easily implemented. I also only just found out about the union keyword in Thrift which might be a good idea for this so I'll have a look but only after I've finished the rest.

          I have no strong opinion about the functions of HBaseAdmin so I'm fine either way. I could also offer an optional AdminThriftServer so that it is separated from the client stuff.

          There is a closeScanner method in there somewhere

          I don't quite understand what you mean with thrift explanation? Do you mean the comment at the beginning of the file explaining what Thrift is? I just kept it from the previous version but I'll work on expanding the package.html too.

          'Text' is gone now, I was wondering about that but forgot to mention it.

          Show
          Lars Francke added a comment - Thanks for the comments! I will have a look at the filters and see which can be easily implemented. I also only just found out about the union keyword in Thrift which might be a good idea for this so I'll have a look but only after I've finished the rest. I have no strong opinion about the functions of HBaseAdmin so I'm fine either way. I could also offer an optional AdminThriftServer so that it is separated from the client stuff. There is a closeScanner method in there somewhere I don't quite understand what you mean with thrift explanation? Do you mean the comment at the beginning of the file explaining what Thrift is? I just kept it from the previous version but I'll work on expanding the package.html too. 'Text' is gone now, I was wondering about that but forgot to mention it.
          Hide
          Bassam Tabbara added a comment -

          It would be great if the outcome of HBASE-2116 could be factored into the new Thrift API.

          I vote for keeping the Admin API given that 1) its already in the existing thrift API and 2) it would be useful for scripting or even unit tests.

          Show
          Bassam Tabbara added a comment - It would be great if the outcome of HBASE-2116 could be factored into the new Thrift API. I vote for keeping the Admin API given that 1) its already in the existing thrift API and 2) it would be useful for scripting or even unit tests.
          Hide
          stack added a comment -

          @Lars Any luck w/ this issue? Is it coming along some?

          Show
          stack added a comment - @Lars Any luck w/ this issue? Is it coming along some?
          Hide
          Lars Francke added a comment -

          I'm sorry for the lack of updates.
          I've not gotten much done over the holidays and just brought myself up to date again - once I'm finished struggling with Ivy I'll get back to work and I'll report back.

          As far as I remember there was nothing particular that I was struggling with in regards to Thrift. Filters and row locking are two things still on my list but I haven't spent much time on them, so I hope to find a solution there.
          As the old Thrift API stays for at least HBase 0.21 before it is deprecated we'll need to update the thrift jar and introduce two new dependencies (SLF4J) to HBase. Those can be removed again once thrift has moved completely to contrib in 0.22 (I presume).

          @Admin API: I think I can offer an option ("--admin") or something like that so that this part becomes optional. Would that be acceptable? One service can extend another so I'd define a ThriftInterace and an ExtendedThriftInterface (with the Admin options).

          @Bassam Tabbara: With unions that should be possible but those suckers aren't documented anywhere and I haven't dug into the code/details for those. I'll just wait what happens with the issue you've mentioned and will try to implement this.

          Show
          Lars Francke added a comment - I'm sorry for the lack of updates. I've not gotten much done over the holidays and just brought myself up to date again - once I'm finished struggling with Ivy I'll get back to work and I'll report back. As far as I remember there was nothing particular that I was struggling with in regards to Thrift. Filters and row locking are two things still on my list but I haven't spent much time on them, so I hope to find a solution there. As the old Thrift API stays for at least HBase 0.21 before it is deprecated we'll need to update the thrift jar and introduce two new dependencies (SLF4J) to HBase. Those can be removed again once thrift has moved completely to contrib in 0.22 (I presume). @Admin API: I think I can offer an option ("--admin") or something like that so that this part becomes optional. Would that be acceptable? One service can extend another so I'd define a ThriftInterace and an ExtendedThriftInterface (with the Admin options). @Bassam Tabbara: With unions that should be possible but those suckers aren't documented anywhere and I haven't dug into the code/details for those. I'll just wait what happens with the issue you've mentioned and will try to implement this.
          Hide
          Tim Sell added a comment -

          About the admin functions. Not sure anyone would bother doing this... but thrift is very flexible, if a client doesn't want the admin functions exposed they can just cut those functions right out of the thrift file before they generate the client code. It'll still work with the server.

          Show
          Tim Sell added a comment - About the admin functions. Not sure anyone would bother doing this... but thrift is very flexible, if a client doesn't want the admin functions exposed they can just cut those functions right out of the thrift file before they generate the client code. It'll still work with the server.
          Hide
          Lars Francke added a comment -

          Just as an update: I've continued working on this today and will try to get a patch up this week. Lars G. can kick me if I don't finish it

          Show
          Lars Francke added a comment - Just as an update: I've continued working on this today and will try to get a patch up this week. Lars G. can kick me if I don't finish it
          Hide
          stack added a comment -

          Thanks for doing this LarsF.

          Show
          stack added a comment - Thanks for doing this LarsF.
          Hide
          Jean-Daniel Cryans added a comment -

          I don't know if it's on your list Lars, but it would be nice when opening scanners to be able to pass a scanner caching value. This would make more sense than having to set a default value in the configuration to make scanning faster, and it would play nice with numRows of getScannerRows.

          Show
          Jean-Daniel Cryans added a comment - I don't know if it's on your list Lars, but it would be nice when opening scanners to be able to pass a scanner caching value. This would make more sense than having to set a default value in the configuration to make scanning faster, and it would play nice with numRows of getScannerRows.
          Hide
          stack added a comment -

          Lars F is on this.

          Show
          stack added a comment - Lars F is on this.
          Hide
          Jonathan Gray added a comment -

          Lars, what's time estimate for this? Should we punt to 0.92?

          Show
          Jonathan Gray added a comment - Lars, what's time estimate for this? Should we punt to 0.92?
          Hide
          Lars Francke added a comment -

          Yes, sorry I won't make it in time for 0.90.

          Show
          Lars Francke added a comment - Yes, sorry I won't make it in time for 0.90.
          Hide
          stack added a comment -

          We'll make do w/ what we have.

          Show
          stack added a comment - We'll make do w/ what we have.
          Hide
          Jonathan Gray added a comment -

          This is starting to get pretty critical. Putting into 0.92 and bumping prio.

          Show
          Jonathan Gray added a comment - This is starting to get pretty critical. Putting into 0.92 and bumping prio.
          Hide
          Lars Francke added a comment -

          I've attached a draft of the new Thrift interface. I'd appreciate any input. But I probably forgot something and there are a few ToDos in there.

          I plan on adding a HbaseAdminClient.thrift file that extends the basic one with all the admin-y stuff (enable/disable tables etc.)

          Show
          Lars Francke added a comment - I've attached a draft of the new Thrift interface. I'd appreciate any input. But I probably forgot something and there are a few ToDos in there. I plan on adding a HbaseAdminClient.thrift file that extends the basic one with all the admin-y stuff (enable/disable tables etc.)
          Hide
          Lars Francke added a comment -

          This adds a patch with a lot of the stuff done. I don't know of any big missing or wrong things with the new Thrift definition file except these:

          • I have not added filters in any form
          • I have not added row locks
          • I added a version constant for the API and set it to 2.0.0 - I thought it might be a good idea to expose a version number that shows if the API has changed and follows the regular version number rules. Haven't added a thrift method yet. Good or bad idea?
          • I'm not entirely sure about TDelete and the delete methods. There are so many different meanings and I haven't managed to put them all in so far. Will have to think about that again

          This also implements the Server part (which are mostly wrappers) and contains the beginnings of an Admin interface (I was too lazy to weed that out from this preview now). Also: Tests not done yet.

          Before going much further I'd love to get some general feedback on the HbaseClient.thrift and Types.thrift file. Does this API work for you or does it need changes?

          Show
          Lars Francke added a comment - This adds a patch with a lot of the stuff done. I don't know of any big missing or wrong things with the new Thrift definition file except these: I have not added filters in any form I have not added row locks I added a version constant for the API and set it to 2.0.0 - I thought it might be a good idea to expose a version number that shows if the API has changed and follows the regular version number rules. Haven't added a thrift method yet. Good or bad idea? I'm not entirely sure about TDelete and the delete methods. There are so many different meanings and I haven't managed to put them all in so far. Will have to think about that again This also implements the Server part (which are mostly wrappers) and contains the beginnings of an Admin interface (I was too lazy to weed that out from this preview now). Also: Tests not done yet. Before going much further I'd love to get some general feedback on the HbaseClient.thrift and Types.thrift file. Does this API work for you or does it need changes?
          Hide
          stack added a comment -

          So, you are going to have a thrift2 package for the new stuff. I don't know of any other way of deprecating the old thrift api while introducing the new. Any ideas on how once we remove the original thrift, of how we'd move thrift2 to thrift package?

          Why would I need an HBaseAdminPool? Why not just create one each time? Why have one at all? Just let thriftians use shell if they want admin functionality?

          IIRC, this does not make a new array each time – it just returns the ByteBuffer backing array:

          +    HTableInterface htable = getTable(table.array());
          

          I like your getFromThrift and getFromHBase namings.

          Oh, nice, you are doing multiget (and multiput)... and incrementColumnValues.

          There are missing licenses on new files and missing copyright header.

          I'd imagine this has to be public for some reason else you'd make it private?

          +  public ThriftServer() {
          

          This is cool:

          +    options.addOption("f", "framed", false, "Use framed transport");
          +    options.addOption("c", "compact", false, "Use the compact protocol");
          

          Just remove this I'd say:

          +  public void testAll() throws Exception {
          ..
          

          Start cluster once rather than per test?

          +  @Before
          +  public void setUp() throws Exception {
          

          Oh, all tests are commented out? Or some of them?

          Hmmm... looks kinda great to me Lars.

          On what I said on IRC, I'd say, we can let this be 0.92. 0.92 is soon after 0.90 so that should help. Meantime anyone who needs new thrift can apply this patch?

          Show
          stack added a comment - So, you are going to have a thrift2 package for the new stuff. I don't know of any other way of deprecating the old thrift api while introducing the new. Any ideas on how once we remove the original thrift, of how we'd move thrift2 to thrift package? Why would I need an HBaseAdminPool? Why not just create one each time? Why have one at all? Just let thriftians use shell if they want admin functionality? IIRC, this does not make a new array each time – it just returns the ByteBuffer backing array: + HTableInterface htable = getTable(table.array()); I like your getFromThrift and getFromHBase namings. Oh, nice, you are doing multiget (and multiput)... and incrementColumnValues. There are missing licenses on new files and missing copyright header. I'd imagine this has to be public for some reason else you'd make it private? + public ThriftServer() { This is cool: + options.addOption( "f" , "framed" , false , "Use framed transport" ); + options.addOption( "c" , "compact" , false , "Use the compact protocol" ); Just remove this I'd say: + public void testAll() throws Exception { .. Start cluster once rather than per test? + @Before + public void setUp() throws Exception { Oh, all tests are commented out? Or some of them? Hmmm... looks kinda great to me Lars. On what I said on IRC, I'd say, we can let this be 0.92. 0.92 is soon after 0.90 so that should help. Meantime anyone who needs new thrift can apply this patch?
          Hide
          Lars Francke added a comment -

          So, you are going to have a thrift2 package for the new stuff. I don't know of any other way of deprecating the old thrift api while introducing the new. Any ideas on how once we remove the original thrift, of how we'd move thrift2 to thrift package?

          That was the easiest option for now but if you have any other ideas? I could throw all that stuff in the old package as well - I don't think there are any name-clashes. The only idea I have for moving the stuff afterwards:

          0.92: Deprecate thrift, introduce thrift2
          0.94: Remove old thrift, deprecate thrift2, copy thrift2 to thrift
          0.96: Remove thrift2

          As I said: I can easily throw it all in the current thrift package I guess.

          Why would I need an HBaseAdminPool? Why not just create one each time? Why have one at all? Just let thriftians use shell if they want admin functionality?

          That class is a remnant from an earlier patch I never finished. Not used at the moment. I think an optional Thrift-Admin interface would be nice but I'll concentrate on the regular client stuff for now.

          IRC, this does not make a new array each time - it just returns the ByteBuffer backing array:

          Yes that's the intention. I know it ain't the most proper way but it should work. Otherwise I'd need to do something like

          ByteBuffer bb = <...>
          byte[] arr = new byte[bb.remaining()];
          bb.get(arr);
          

          or use a CharsetDecoder or CharBuffer or something like that. I'm really not sure how to best handle ByteBuffers. So any hint would be great.

          There are missing licenses on new files and missing copyright header.

          Thanks will fix that!

          I'd imagine this has to be public for some reason else you'd make it private?

          No that was just an oversight as far as I can see. Thanks, will look at it.

          The rest you mentioned is not finished yet - it's just a literal copy & paste from the old code (that's why the tests are commented out). I didn't change a thing yet. But it's on my list.

          I just thought it was about time I finally showed some results

          And sure this can go into 0.92 - it has very little dependencies on the rest of the code so I'll keep the patch updated if needed.

          Show
          Lars Francke added a comment - So, you are going to have a thrift2 package for the new stuff. I don't know of any other way of deprecating the old thrift api while introducing the new. Any ideas on how once we remove the original thrift, of how we'd move thrift2 to thrift package? That was the easiest option for now but if you have any other ideas? I could throw all that stuff in the old package as well - I don't think there are any name-clashes. The only idea I have for moving the stuff afterwards: 0.92: Deprecate thrift, introduce thrift2 0.94: Remove old thrift, deprecate thrift2, copy thrift2 to thrift 0.96: Remove thrift2 As I said: I can easily throw it all in the current thrift package I guess. Why would I need an HBaseAdminPool? Why not just create one each time? Why have one at all? Just let thriftians use shell if they want admin functionality? That class is a remnant from an earlier patch I never finished. Not used at the moment. I think an optional Thrift-Admin interface would be nice but I'll concentrate on the regular client stuff for now. IRC, this does not make a new array each time - it just returns the ByteBuffer backing array: Yes that's the intention. I know it ain't the most proper way but it should work. Otherwise I'd need to do something like ByteBuffer bb = <...> byte [] arr = new byte [bb.remaining()]; bb.get(arr); or use a CharsetDecoder or CharBuffer or something like that. I'm really not sure how to best handle ByteBuffers. So any hint would be great. There are missing licenses on new files and missing copyright header. Thanks will fix that! I'd imagine this has to be public for some reason else you'd make it private? No that was just an oversight as far as I can see. Thanks, will look at it. The rest you mentioned is not finished yet - it's just a literal copy & paste from the old code (that's why the tests are commented out). I didn't change a thing yet. But it's on my list. I just thought it was about time I finally showed some results And sure this can go into 0.92 - it has very little dependencies on the rest of the code so I'll keep the patch updated if needed.
          Hide
          ryan rawson added a comment -

          i would be against the thrift2 and package moving stuff. It would require our users to start a particular version of thrift depending on which one they wanted, and would make it difficult for users to migrate, unless you also came up with a way to run both .thrift files in 1 server.

          Thrift doesnt appear to offer any @deprecated tags, so we are probably going to have to do this the old fashioned way.

          But even so, I think the timeline to remove the old API is pretty aggressive. We removed BatchUpdate fairly quickly, and unless there is a compelling reason I'd rather keep old APIs around just a bit longer. Makes transition easier since it gives users more time.

          Show
          ryan rawson added a comment - i would be against the thrift2 and package moving stuff. It would require our users to start a particular version of thrift depending on which one they wanted, and would make it difficult for users to migrate, unless you also came up with a way to run both .thrift files in 1 server. Thrift doesnt appear to offer any @deprecated tags, so we are probably going to have to do this the old fashioned way. But even so, I think the timeline to remove the old API is pretty aggressive. We removed BatchUpdate fairly quickly, and unless there is a compelling reason I'd rather keep old APIs around just a bit longer. Makes transition easier since it gives users more time.
          Hide
          Lars Francke added a comment -

          Services can extend each other so I can - in theory - extend the old API so both would be available in one server but I don't know if that is really a good option. I don't think it would be a technical problem though. The APIs are so different - I think they share some exceptions but that's it (and those are fine). Come to think of it: Both have a get(...) method if I remember correctly (Thrift does not know overloaded methods) so we'd have to rename one of the two.

          Yeah unfortunately Thrift does not have any options for deprecating stuff so we'll just have to communicate it prominently.

          The timeline I posted up there was purely fictional. Nothing stops us from keeping both APIs around. The maintenance overhead so far isn't that bad as far as I can tell. But we should make it very clear that one of the two is probably going away at some point to avoid confusion.

          Show
          Lars Francke added a comment - Services can extend each other so I can - in theory - extend the old API so both would be available in one server but I don't know if that is really a good option. I don't think it would be a technical problem though. The APIs are so different - I think they share some exceptions but that's it (and those are fine). Come to think of it: Both have a get(...) method if I remember correctly (Thrift does not know overloaded methods) so we'd have to rename one of the two. Yeah unfortunately Thrift does not have any options for deprecating stuff so we'll just have to communicate it prominently. The timeline I posted up there was purely fictional. Nothing stops us from keeping both APIs around. The maintenance overhead so far isn't that bad as far as I can tell. But we should make it very clear that one of the two is probably going away at some point to avoid confusion.
          Hide
          Bryan Duxbury added a comment -

          Stack asked me to take a look at your Thrift IDL. Here are the comments I came up with:

          • It's a little confusing to typedef binary to Bytes, but that's all cosmetic.
          • All your files (types, admin, client) use the same java namespace. This means they'll all generate a Constants.java with the same path, which means 2/3 of them will be clobbered. You only have constants in one of the files, which is good, but I think there's a good chance that class will be destroyed.

          Otherwise, looks good to me.

          Show
          Bryan Duxbury added a comment - Stack asked me to take a look at your Thrift IDL. Here are the comments I came up with: It's a little confusing to typedef binary to Bytes, but that's all cosmetic. All your files (types, admin, client) use the same java namespace. This means they'll all generate a Constants.java with the same path, which means 2/3 of them will be clobbered. You only have constants in one of the files, which is good, but I think there's a good chance that class will be destroyed. Otherwise, looks good to me.
          Hide
          Lars Francke added a comment -

          Oh great thanks Bryan (and Stack) for the review!

          • The typedef to Binary is just carried over from the old API, I thought it would be good to not confuse to much by suddenly changing that as well but you're probably right so I guess I'll change that as well.
          • I didn't even know about the Constants.java thing, thanks. I'll just introduce new namespaces. Is that a problem only with Java or the other languages as well?

          I'll have those changes in the next patch.

          Show
          Lars Francke added a comment - Oh great thanks Bryan (and Stack) for the review! The typedef to Binary is just carried over from the old API, I thought it would be good to not confuse to much by suddenly changing that as well but you're probably right so I guess I'll change that as well. I didn't even know about the Constants.java thing, thanks. I'll just introduce new namespaces. Is that a problem only with Java or the other languages as well? I'll have those changes in the next patch.
          Hide
          stack added a comment -

          @Lars Any chance of your updating this patch? I'd like to get it into TRUNK for 0.92. Good on you.

          Show
          stack added a comment - @Lars Any chance of your updating this patch? I'd like to get it into TRUNK for 0.92. Good on you.
          Hide
          Lars Francke added a comment -

          I'll get on it next week.

          Show
          Lars Francke added a comment - I'll get on it next week.
          Hide
          Lars George added a comment -

          Haggling with LarsF to get this done in trunk. I suggest we use the Stargate/REST filter language as per HBASE-1696 since then it is cross connector compatible and it does not really matter anyways for either to have a non text based filter definition.

          Show
          Lars George added a comment - Haggling with LarsF to get this done in trunk. I suggest we use the Stargate/REST filter language as per HBASE-1696 since then it is cross connector compatible and it does not really matter anyways for either to have a non text based filter definition.
          Hide
          Lars George added a comment -

          Quick note: ThriftHbaseClientHandler should match the classname ThriftHBaseClientHandler. There are Apache licenses missing.

          Show
          Lars George added a comment - Quick note: ThriftHbaseClientHandler should match the classname ThriftHBaseClientHandler. There are Apache licenses missing.
          Hide
          Lars George added a comment -

          We also need to add support for increment().

          Show
          Lars George added a comment - We also need to add support for increment().
          Hide
          ryan rawson added a comment -

          talking about this recently with stack, I made the point that we should probably put the new API in the same package/thrift file as the old. Use new symbols, at the end of the .thrift file. That way people can use new and old clients with the same server, making migration easier. It might make some names a little ugly (we could go with the windowsy Ex suffix?) but making an easier migration strategy is important I feel.

          Show
          ryan rawson added a comment - talking about this recently with stack, I made the point that we should probably put the new API in the same package/thrift file as the old. Use new symbols, at the end of the .thrift file. That way people can use new and old clients with the same server, making migration easier. It might make some names a little ugly (we could go with the windowsy Ex suffix?) but making an easier migration strategy is important I feel.
          Hide
          Lars George added a comment -

          There is also the duplicate HBASE-3513. We also need to go to Thrift 0.6.0 to not trail again.

          Show
          Lars George added a comment - There is also the duplicate HBASE-3513 . We also need to go to Thrift 0.6.0 to not trail again.
          Hide
          Lars Francke added a comment -

          I'm afraid I will finally have to say that I have no idea when I'll be able to work on this. I've promised it for a long time but if anyone else wants to take a stab at it feel free.

          I'm pretty confident in the Thrift definition file I've come up with. It needs some minor updates for the updated API (increment for example) and the delete method is still missing one of the possible cases (I've documented it in the patch) and filters are not implemented yet either.

          I'll gladly answer any questions about this in case anyone wants to work on it and if I have the time I'll improve the patch but I can't promise anything, sorry

          Show
          Lars Francke added a comment - I'm afraid I will finally have to say that I have no idea when I'll be able to work on this. I've promised it for a long time but if anyone else wants to take a stab at it feel free. I'm pretty confident in the Thrift definition file I've come up with. It needs some minor updates for the updated API (increment for example) and the delete method is still missing one of the possible cases (I've documented it in the patch) and filters are not implemented yet either. I'll gladly answer any questions about this in case anyone wants to work on it and if I have the time I'll improve the patch but I can't promise anything, sorry
          Hide
          Lars Francke added a comment -

          About the comments from Bryan: I've removed the Bytes typedef but I haven't yet dealt with the namespace issue concerning Constants.java

          Apart from that increment, batch and filters are missing as well as the KeyValue.Type.DeleteColumn semantic in the delete methods. I also didn't do any tests so far and I didn't move any of the new stuff in the old file as per Ryan's latest comment.

          I'd favor keeping it separate instead of mangling names etc. but that's easy for me to say without having to do a migration.

          I've attached the latest version of what I've done.

          Show
          Lars Francke added a comment - About the comments from Bryan: I've removed the Bytes typedef but I haven't yet dealt with the namespace issue concerning Constants.java Apart from that increment, batch and filters are missing as well as the KeyValue.Type.DeleteColumn semantic in the delete methods. I also didn't do any tests so far and I didn't move any of the new stuff in the old file as per Ryan's latest comment. I'd favor keeping it separate instead of mangling names etc. but that's easy for me to say without having to do a migration. I've attached the latest version of what I've done.
          Hide
          Lars Francke added a comment -

          One last thing I forgot: These files are generated using the Thrift 0.6.0 compiler.

          Show
          Lars Francke added a comment - One last thing I forgot: These files are generated using the Thrift 0.6.0 compiler.
          Hide
          stack added a comment -

          @Lars Thanks for the work done so far. One of us will take it to the finish line. Good on you.

          Show
          stack added a comment - @Lars Thanks for the work done so far. One of us will take it to the finish line. Good on you.
          Show
          Lars Francke added a comment - https://issues.apache.org/jira/browse/THRIFT-363?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13024799#comment-13024799 Thrift is finally in the Maven repository!
          Hide
          stack added a comment -

          @Lars Some fellas figured it ahead of you (smile). See https://mail.google.com/mail/?shva=1#label/hbase-issues/12f8cf7db44d14fb

          Show
          stack added a comment - @Lars Some fellas figured it ahead of you (smile). See https://mail.google.com/mail/?shva=1#label/hbase-issues/12f8cf7db44d14fb
          Hide
          Ted Yu added a comment -

          I tried to apply latest patch on TRUNK.
          It doesn't compile:

          /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftHBaseClientHandler.java:[41,23] unreported exception java.io.IOException; must be caught or declared to be thrown
          
          /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[135,17] cannot find symbol
          symbol  : constructor TNonblockingServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TNonblockingServerTransport,org.apache.thrift.transport.TFramedTransport.Factory,org.apache.thrift.protocol.TProtocolFactory)
          location: class org.apache.thrift.server.TNonblockingServer
          
          /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[138,17] cannot find symbol
          symbol  : constructor THsHaServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TNonblockingServerTransport,org.apache.thrift.transport.TFramedTransport.Factory,org.apache.thrift.protocol.TProtocolFactory)
          location: class org.apache.thrift.server.THsHaServer
          
          /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[165,15] cannot find symbol
          symbol  : constructor TThreadPoolServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TServerTransport,org.apache.thrift.transport.TTransportFactory,org.apache.thrift.protocol.TProtocolFactory)
          location: class org.apache.thrift.server.TThreadPoolServer
          

          @Lars, can you take a look ?

          Thanks

          Show
          Ted Yu added a comment - I tried to apply latest patch on TRUNK. It doesn't compile: /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftHBaseClientHandler.java:[41,23] unreported exception java.io.IOException; must be caught or declared to be thrown /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[135,17] cannot find symbol symbol : constructor TNonblockingServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TNonblockingServerTransport,org.apache.thrift.transport.TFramedTransport.Factory,org.apache.thrift.protocol.TProtocolFactory) location: class org.apache.thrift.server.TNonblockingServer /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[138,17] cannot find symbol symbol : constructor THsHaServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TNonblockingServerTransport,org.apache.thrift.transport.TFramedTransport.Factory,org.apache.thrift.protocol.TProtocolFactory) location: class org.apache.thrift.server.THsHaServer /home/hadoop/hbase/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java:[165,15] cannot find symbol symbol : constructor TThreadPoolServer(org.apache.hadoop.hbase.thrift2.generated.HbaseClient.Processor,org.apache.thrift.transport.TServerTransport,org.apache.thrift.transport.TTransportFactory,org.apache.thrift.protocol.TProtocolFactory) location: class org.apache.thrift.server.TThreadPoolServer @Lars, can you take a look ? Thanks
          Hide
          Tim Sell added a comment -

          I have been working on this, continuing work I did at the berlin buzzwords hbase hackday.
          https://github.com/tims/hbase-thrift

          I'll make a patch the fixes I made to Lars stuff and upload it here asap.

          Show
          Tim Sell added a comment - I have been working on this, continuing work I did at the berlin buzzwords hbase hackday. https://github.com/tims/hbase-thrift I'll make a patch the fixes I made to Lars stuff and upload it here asap.
          Hide
          Ted Yu added a comment -

          @Tim, any update ?

          Show
          Ted Yu added a comment - @Tim, any update ?
          Hide
          Tim Sell added a comment -

          Added a new patch.
          This should apply cleanly to trunk and compile.

          I've haven't written unit tests for it, just checked most parts by hand and fixed things.

          You can start thrift and thrift2 servers separately using the daemon scripts.

          Generated code is via Thrift version 0.6.1.

          Missing some licence headers.

          Show
          Tim Sell added a comment - Added a new patch. This should apply cleanly to trunk and compile. I've haven't written unit tests for it, just checked most parts by hand and fixed things. You can start thrift and thrift2 servers separately using the daemon scripts. Generated code is via Thrift version 0.6.1. Missing some licence headers.
          Hide
          Ted Yu added a comment -

          Sorry to get back to this so late.

          I got the following:

          [INFO] Compilation failure
          
          /home/hadoop/hbase/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftServer.java:[68,19] ThriftServer() has private access in org.apache.hadoop.hbase.thrift2.ThriftServer
          
          /home/hadoop/hbase/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftServer.java:[83,22] cannot find symbol
          symbol  : method isMasterRunning()
          location: class org.apache.hadoop.hbase.thrift2.ThriftHBaseServiceHandler
          
          Show
          Ted Yu added a comment - Sorry to get back to this so late. I got the following: [INFO] Compilation failure /home/hadoop/hbase/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftServer.java:[68,19] ThriftServer() has private access in org.apache.hadoop.hbase.thrift2.ThriftServer /home/hadoop/hbase/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftServer.java:[83,22] cannot find symbol symbol : method isMasterRunning() location: class org.apache.hadoop.hbase.thrift2.ThriftHBaseServiceHandler
          Hide
          Ted Yu added a comment -

          I made ThriftServer package private so that the test compiles. But bin/hbase uses org.apache.hadoop.hbase.thrift2.ThriftServer, I guess it should be public.
          I don't see isMasterRunning() in ThriftHBaseServiceHandler so I commented out the call in TestThriftServer

          The new test runs much faster than the existing one:

          Running org.apache.hadoop.hbase.thrift2.TestThriftServer
          Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.213 sec
          Running org.apache.hadoop.hbase.thrift.TestThriftServer
          Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 36.776 sec
          

          Maybe I am missing something.

          Show
          Ted Yu added a comment - I made ThriftServer package private so that the test compiles. But bin/hbase uses org.apache.hadoop.hbase.thrift2.ThriftServer, I guess it should be public. I don't see isMasterRunning() in ThriftHBaseServiceHandler so I commented out the call in TestThriftServer The new test runs much faster than the existing one: Running org.apache.hadoop.hbase.thrift2.TestThriftServer Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.213 sec Running org.apache.hadoop.hbase.thrift.TestThriftServer Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 36.776 sec Maybe I am missing something.
          Hide
          Ted Yu added a comment -

          TestThriftServer.testAll() is commented out.
          Same with the tests it is supposed to run.
          Please finish TestThriftServer.

          Show
          Ted Yu added a comment - TestThriftServer.testAll() is commented out. Same with the tests it is supposed to run. Please finish TestThriftServer.
          Hide
          Ted Yu added a comment -

          There seems to be little attention to this JIRA.

          Show
          Ted Yu added a comment - There seems to be little attention to this JIRA.
          Hide
          Bob Copeland added a comment -

          Hi,

          I put some fixes for the above issues Ted Yu noted and another fix in my git tree here (will send Tim S. a pull request in a sec):

          http://github.com/bcopeland/hbase-thrift

          The other fix was to allow timeRange to be null in scannerOpen's TScan.

          (I work on a small python library that wraps the thrift API. I just forward ported it to thrift2 today and these were the first issues to pop up.)

          Show
          Bob Copeland added a comment - Hi, I put some fixes for the above issues Ted Yu noted and another fix in my git tree here (will send Tim S. a pull request in a sec): http://github.com/bcopeland/hbase-thrift The other fix was to allow timeRange to be null in scannerOpen's TScan. (I work on a small python library that wraps the thrift API. I just forward ported it to thrift2 today and these were the first issues to pop up.)
          Hide
          Bob Copeland added a comment -

          Also, seems checkAndPut() should allow value=null like checkAndDelete() – I also made this change on my branch.

          Show
          Bob Copeland added a comment - Also, seems checkAndPut() should allow value=null like checkAndDelete() – I also made this change on my branch.
          Hide
          Tim Sell added a comment -

          Added patch with Bob Copeland's fixes and updated to trunk

          Show
          Tim Sell added a comment - Added patch with Bob Copeland's fixes and updated to trunk
          Hide
          Tim Sell added a comment -

          Thanks Bob,
          my patch still doesn't have tests, looking at that now.

          Show
          Tim Sell added a comment - Thanks Bob, my patch still doesn't have tests, looking at that now.
          Hide
          Ted Yu added a comment -

          I applied patch v4 and got the following:
          http://pastebin.com/rmpXff7m

          Show
          Ted Yu added a comment - I applied patch v4 and got the following: http://pastebin.com/rmpXff7m
          Hide
          Tim Sell added a comment -

          Weird. Strange it works for me, turns out I am compiling with thrift 0.6.1 and hbase is using 0.7,

          cancelling the patch.

          I'll stick a new one up in a bit.

          Show
          Tim Sell added a comment - Weird. Strange it works for me, turns out I am compiling with thrift 0.6.1 and hbase is using 0.7, cancelling the patch. I'll stick a new one up in a bit.
          Hide
          Tim Sell added a comment -

          Added patch which has the thrift2 generated files generated from 0.7.0,

          Also has a few tests.

          Show
          Tim Sell added a comment - Added patch which has the thrift2 generated files generated from 0.7.0, Also has a few tests.
          Hide
          Ted Yu added a comment -

          TestThriftHBaseServiceHandler passed for patch v5.

          Can we change the wording for:

          +  echo "  thrift2          run the new HBase Thrift server"
          

          I believe there would be newer Thrift server down the road

          Also, experience using thrift2 would be helpful for other users.

          Show
          Ted Yu added a comment - TestThriftHBaseServiceHandler passed for patch v5. Can we change the wording for: + echo " thrift2 run the new HBase Thrift server" I believe there would be newer Thrift server down the road Also, experience using thrift2 would be helpful for other users.
          Hide
          Tim Sell added a comment -

          Is "run the HBase Thrift2 server" ok?

          What do you mean by experience? Do you mean examples of usage in different languages?

          Show
          Tim Sell added a comment - Is "run the HBase Thrift2 server" ok? What do you mean by experience? Do you mean examples of usage in different languages?
          Hide
          Ted Yu added a comment -

          "run the HBase Thrift2 server" is fine.
          By experience, usage in different programming languages would be nice to share.

          Show
          Ted Yu added a comment - "run the HBase Thrift2 server" is fine. By experience, usage in different programming languages would be nice to share.
          Hide
          Tim Sell added a comment -

          Added new patch, with a few more tests and a incomplete trivial java example.

          Show
          Tim Sell added a comment - Added new patch, with a few more tests and a incomplete trivial java example.
          Hide
          Ted Yu added a comment -

          src/examples/thrift2/DemoClient.java needs license.

          Show
          Ted Yu added a comment - src/examples/thrift2/DemoClient.java needs license.
          Hide
          Tim Sell added a comment -

          new patch, added licenses and trivial python demo.

          Show
          Tim Sell added a comment - new patch, added licenses and trivial python demo.
          Hide
          Ted Yu added a comment -

          +1 on patch v7.

          Show
          Ted Yu added a comment - +1 on patch v7.
          Hide
          Bob Copeland added a comment -

          I think it would be useful to take care of this todo:

          + * TODO: This is missing the KeyValue.Type.DeleteColumn semantic. I could add a DeleteType or something like that

          Right now thrift2 is using deleteColumn() instead of deleteColumns() which I find to be the least useful of the two (due to re-appearing old versions). I made a candidate incremental patch on my tree adding a TDeleteType following the TODO's hint. It's kind of ugly but works for me. I would also be happy with just using deleteColumns() by default.

          Show
          Bob Copeland added a comment - I think it would be useful to take care of this todo: + * TODO: This is missing the KeyValue.Type.DeleteColumn semantic. I could add a DeleteType or something like that Right now thrift2 is using deleteColumn() instead of deleteColumns() which I find to be the least useful of the two (due to re-appearing old versions). I made a candidate incremental patch on my tree adding a TDeleteType following the TODO's hint. It's kind of ugly but works for me. I would also be happy with just using deleteColumns() by default.
          Hide
          Bob Copeland added a comment -

          Incremental patch adding TDeleteType enum

          Show
          Bob Copeland added a comment - Incremental patch adding TDeleteType enum
          Hide
          Ted Yu added a comment -

          @Bob:
          Can you combine your patch with Tim's patch v7 ?
          Thanks

          Show
          Ted Yu added a comment - @Bob: Can you combine your patch with Tim's patch v7 ? Thanks
          Hide
          Bob Copeland added a comment -

          Sure – v8 is v7 + the TDeleteType addition.

          Show
          Bob Copeland added a comment - Sure – v8 is v7 + the TDeleteType addition.
          Hide
          Tim Sell added a comment -

          Looks good. I added another patch which also sets TDelete to use DELETE_COLUMNS by default and added some tests.

          Show
          Tim Sell added a comment - Looks good. I added another patch which also sets TDelete to use DELETE_COLUMNS by default and added some tests.
          Hide
          stack added a comment -

          Fancy, you have democlient for thrift2 too!

          Thanks for including a package.html. Should be more forceful about thrift one being deprecated but I can fix on commit.

          It doesn't look like much overlap between thrift2 utilities and the other thrift utilities if any. Thats good.

          There are a bunch of spelling mistakes in the patch. I can fix on commit (And I though you were in skuhl Tim!) – "Size of pool configuraple"

          Bob and Tim, you fellas +1 on committing this latest patch. Do tests pass? The new thrift2 tests? LarsF you have an opinion?

          It looks good to me. If you fellas are good, will commit over next few days.

          Show
          stack added a comment - Fancy, you have democlient for thrift2 too! Thanks for including a package.html. Should be more forceful about thrift one being deprecated but I can fix on commit. It doesn't look like much overlap between thrift2 utilities and the other thrift utilities if any. Thats good. There are a bunch of spelling mistakes in the patch. I can fix on commit (And I though you were in skuhl Tim!) – "Size of pool configuraple" Bob and Tim, you fellas +1 on committing this latest patch. Do tests pass? The new thrift2 tests? LarsF you have an opinion? It looks good to me. If you fellas are good, will commit over next few days.
          Hide
          Tim Sell added a comment -

          Well, Lars did a lot of the work and deserves credit, so not all the typos are my fault

          All the demo clients do is a single put and get. The aren't as standalone as the current thrift package because thrift2 doesn't have a createTable method, as we are matching HTable only.

          The thrift2 tests pass, I haven't run all the hbase tests. I thought "submit patch" would do that but it hasn't any of the times I've used it.

          +1 on committing this latest patch (with typos fixed).

          Show
          Tim Sell added a comment - Well, Lars did a lot of the work and deserves credit, so not all the typos are my fault All the demo clients do is a single put and get. The aren't as standalone as the current thrift package because thrift2 doesn't have a createTable method, as we are matching HTable only. The thrift2 tests pass, I haven't run all the hbase tests. I thought "submit patch" would do that but it hasn't any of the times I've used it. +1 on committing this latest patch (with typos fixed).
          Hide
          Bob Copeland added a comment -

          +1 from me.

          Just tested with latest patch on our cluster, and it passes pybase's (small, not at all exhaustive) test suite which does some puts, gets, deletes, a checkAndPut, checkAndDelete, and a range scan with the scanner methods.

          Show
          Bob Copeland added a comment - +1 from me. Just tested with latest patch on our cluster, and it passes pybase's (small, not at all exhaustive) test suite which does some puts, gets, deletes, a checkAndPut, checkAndDelete, and a range scan with the scanner methods.
          Hide
          stack added a comment -

          Submitting patch to see if it breaks any unit tests.

          Show
          stack added a comment - Submitting patch to see if it breaks any unit tests.
          Hide
          Jonathan Gray added a comment -

          One more requested change. Over in HBASE-4658 the map of attributes was added to the available APIs in thrift. Could we add this to the new TScan, TGet, etc. structs?

          Show
          Jonathan Gray added a comment - One more requested change. Over in HBASE-4658 the map of attributes was added to the available APIs in thrift. Could we add this to the new TScan, TGet, etc. structs?
          Hide
          Tim Sell added a comment -

          I think that should be a follow up issue to this rather than a block.

          Show
          Tim Sell added a comment - I think that should be a follow up issue to this rather than a block.
          Hide
          Tim Sell added a comment -

          I don't think the patch is getting submitted to jenkins.

          Show
          Tim Sell added a comment - I don't think the patch is getting submitted to jenkins.
          Hide
          Ted Yu added a comment -

          Patch version 10 is same as patch v9.
          Generated for patch testing.

          Show
          Ted Yu added a comment - Patch version 10 is same as patch v9. Generated for patch testing.
          Hide
          Ted Yu added a comment -

          @Tim:
          HadoopQA won't recognize patch v9 which requires -p1 at time of application.

          I regenerated patch v10.
          Once test result comes back clean, I can integrate.

          Show
          Ted Yu added a comment - @Tim: HadoopQA won't recognize patch v9 which requires -p1 at time of application. I regenerated patch v10. Once test result comes back clean, I can integrate.
          Hide
          Ted Yu added a comment -

          Previous patch testing was blocked by broken TRUNK builds.

          Show
          Ted Yu added a comment - Previous patch testing was blocked by broken TRUNK builds.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12501880/1744-trunk.10
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated -165 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 40 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/130//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/130//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/130//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501880/1744-trunk.10 against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -165 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 40 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/130//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/130//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/130//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          @Tim:
          Do you want to take care of the following ?

          [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java:[108,28] unreported exception java.lang.Exception; must be caught or declared to be thrown
          
          Show
          Ted Yu added a comment - @Tim: Do you want to take care of the following ? [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java:[108,28] unreported exception java.lang.Exception; must be caught or declared to be thrown
          Hide
          Tim Sell added a comment -

          cancelling existing patch

          Show
          Tim Sell added a comment - cancelling existing patch
          Hide
          Tim Sell added a comment -

          added patch with fix so tests build against current trunk (also now using --no-prefix option in git diff)

          Show
          Tim Sell added a comment - added patch with fix so tests build against current trunk (also now using --no-prefix option in git diff)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12501883/HBASE-1744.11.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated -165 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 40 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.master.TestMasterFailover
          org.apache.hadoop.hbase.master.TestDistributedLogSplitting

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/131//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/131//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/131//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501883/HBASE-1744.11.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -165 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 40 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestMasterFailover org.apache.hadoop.hbase.master.TestDistributedLogSplitting Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/131//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/131//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/131//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Failed tested were caused by 'Too many open files'

          Show
          Ted Yu added a comment - Failed tested were caused by 'Too many open files'
          Hide
          stack added a comment -

          +1 on commit to TRUNK.

          Show
          stack added a comment - +1 on commit to TRUNK.
          Hide
          Ted Yu added a comment -

          Integrated to TRUNK.

          Thanks for the patch, Lars and Tim.

          Thanks for the green light Stack.

          Show
          Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch, Lars and Tim. Thanks for the green light Stack.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2399 (See https://builds.apache.org/job/HBase-TRUNK/2399/)
          HBASE-1744 Thrift server to match the new java api (Tim Sell)

          tedyu :
          Files :

          • /hbase/trunk/CHANGES.txt
          • /hbase/trunk/bin/hbase
          • /hbase/trunk/src/examples/thrift2
          • /hbase/trunk/src/examples/thrift2/DemoClient.java
          • /hbase/trunk/src/examples/thrift2/DemoClient.py
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftHBaseServiceHandler.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumn.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumnIncrement.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumnValue.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDeleteType.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TGet.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/THBaseService.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIOError.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIllegalArgument.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TResult.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TScan.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TTimeRange.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/package.html
          • /hbase/trunk/src/main/resources/org/apache/hadoop/hbase/thrift2
          • /hbase/trunk/src/main/resources/org/apache/hadoop/hbase/thrift2/hbase.thrift
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2399 (See https://builds.apache.org/job/HBase-TRUNK/2399/ ) HBASE-1744 Thrift server to match the new java api (Tim Sell) tedyu : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/bin/hbase /hbase/trunk/src/examples/thrift2 /hbase/trunk/src/examples/thrift2/DemoClient.java /hbase/trunk/src/examples/thrift2/DemoClient.py /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2 /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftHBaseServiceHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumn.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumnIncrement.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TColumnValue.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDeleteType.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TGet.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/THBaseService.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIOError.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIllegalArgument.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TResult.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TScan.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TTimeRange.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift2/package.html /hbase/trunk/src/main/resources/org/apache/hadoop/hbase/thrift2 /hbase/trunk/src/main/resources/org/apache/hadoop/hbase/thrift2/hbase.thrift /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2 /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
          Hide
          Ted Yu added a comment -

          Addendum that makes TestThriftHBaseServiceHandler immune to hanging minicluster.

          Show
          Ted Yu added a comment - Addendum that makes TestThriftHBaseServiceHandler immune to hanging minicluster.
          Hide
          Ted Yu added a comment -

          From https://builds.apache.org/view/G-L/view/HBase/job/HBase-TRUNK/2399/artifact/trunk/target/surefire-reports/org.apache.hadoop.hbase.thrift2.TestThriftHBaseServiceHandler-output.txt:

          2011-11-02 09:18:25,930 INFO  [main] zookeeper.MiniZooKeeperCluster(141): Failed binding ZK Server to client port: 21818
          2011-11-02 09:18:25,958 INFO  [main] zookeeper.MiniZooKeeperCluster(164): Started MiniZK Cluster and connect 1 ZK server on client port: 21819
          

          Let's see what happens in build 2400.

          Show
          Ted Yu added a comment - From https://builds.apache.org/view/G-L/view/HBase/job/HBase-TRUNK/2399/artifact/trunk/target/surefire-reports/org.apache.hadoop.hbase.thrift2.TestThriftHBaseServiceHandler-output.txt: 2011-11-02 09:18:25,930 INFO [main] zookeeper.MiniZooKeeperCluster(141): Failed binding ZK Server to client port: 21818 2011-11-02 09:18:25,958 INFO [main] zookeeper.MiniZooKeeperCluster(164): Started MiniZK Cluster and connect 1 ZK server on client port: 21819 Let's see what happens in build 2400.
          Hide
          Ted Yu added a comment -

          The test passed in build 2400.

          Running org.apache.hadoop.hbase.thrift2.TestThriftHBaseServiceHandler
          Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.203 sec
          
          Show
          Ted Yu added a comment - The test passed in build 2400. Running org.apache.hadoop.hbase.thrift2.TestThriftHBaseServiceHandler Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.203 sec
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2400 (See https://builds.apache.org/job/HBase-TRUNK/2400/)
          HBASE-1744 HBaseAdmin ctor should obtain Configuration from HBaseTestingUtility

          tedyu :
          Files :

          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2400 (See https://builds.apache.org/job/HBase-TRUNK/2400/ ) HBASE-1744 HBaseAdmin ctor should obtain Configuration from HBaseTestingUtility tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java

            People

            • Assignee:
              Tim Sell
              Reporter:
              Tim Sell
            • Votes:
              1 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development