HBase
  1. HBase
  2. HBASE-2400

new connector for Avro RPC access to HBase cluster

    Details

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

      Description

      Build a new connector contrib architecturally equivalent to the Thrift connector, but using Avro serialization and associated transport and RPC server work. Support AAA (audit, authentication, authorization).

      1. HBASE-2400-v0.patch
        128 kB
        Jeff Hammerbacher

        Issue Links

          Activity

          Hide
          Doug Cutting added a comment -

          Why not just use Avro over HTTP for now? That would give you something usable today. When Avro specifies a higher-performance, secure, etc. transport and that's implemented in all languages, then you can upgrade to that. But a Jetty-based service using Avro's existing ResponderServlet would provide Avro-based access for Java, Ruby, Python and C today.

          Show
          Doug Cutting added a comment - Why not just use Avro over HTTP for now? That would give you something usable today. When Avro specifies a higher-performance, secure, etc. transport and that's implemented in all languages, then you can upgrade to that. But a Jetty-based service using Avro's existing ResponderServlet would provide Avro-based access for Java, Ruby, Python and C today.
          Hide
          Andrew Purtell added a comment -

          @Doug: Thanks for the suggestion.

          Minor update on description as we are trying to do away with the notion of contrib.

          Show
          Andrew Purtell added a comment - @Doug: Thanks for the suggestion. Minor update on description as we are trying to do away with the notion of contrib.
          Hide
          Jeff Hammerbacher added a comment -

          Hey,

          I'm getting a little something started at http://github.com/hammer/hbase-avro. It's absolutely hideous for now, but I have TCell, IOError, and get() implemented.

          My first goal is to get this server on par with the existing Thrift server. That should be possible in the next week or two. Once that is complete, we'll move on to updating the service interface to match the new APIs (especially filters and admin stuff, see https://issues.apache.org/jira/browse/HBASE-1744). Once the interface update is complete, we can worry about security.

          Let me know if you currently use the Thrift interface, as I'd love your feedback as I push out more bits to play with.

          Later,
          Jeff

          Show
          Jeff Hammerbacher added a comment - Hey, I'm getting a little something started at http://github.com/hammer/hbase-avro . It's absolutely hideous for now, but I have TCell, IOError, and get() implemented. My first goal is to get this server on par with the existing Thrift server. That should be possible in the next week or two. Once that is complete, we'll move on to updating the service interface to match the new APIs (especially filters and admin stuff, see https://issues.apache.org/jira/browse/HBASE-1744 ). Once the interface update is complete, we can worry about security. Let me know if you currently use the Thrift interface, as I'd love your feedback as I push out more bits to play with. Later, Jeff
          Hide
          Andrew Purtell added a comment -

          Cool, have at it!

          Show
          Andrew Purtell added a comment - Cool, have at it!
          Hide
          Jeff Hammerbacher added a comment -

          Ported my work to trunk, try it out: http://github.com/hammer/hbase-trunk-with-avro.

          Will fill in the rest of the methods later this week, hope to have a patch available by this weekend.

          Show
          Jeff Hammerbacher added a comment - Ported my work to trunk, try it out: http://github.com/hammer/hbase-trunk-with-avro . Will fill in the rest of the methods later this week, hope to have a patch available by this weekend.
          Hide
          Jeff Hammerbacher added a comment -

          Just to heartbeat here: I have completed the implementation of the methods listed below, which makes the server feature complete for my first cut (I've avoided the surgery commands, as it seems like a bad idea to let these commands be run from an Avro client. I've also avoided the atomic increment stuff, though that might make it into the patch). I'm going to do a refactoring pass some time next week. Does anyone have a strong interest in reviewing this code? Andrew?

          void createTable(ATableDescriptor table) throws AIOError, AIllegalArgument, ATableExists, AMasterNotRunning;
          void deleteTable(bytes table) throws AIOError;
          void modifyTable(bytes table, ATableDescriptor tableDescriptor) throws AIOError;

          void addFamily(bytes table, AFamilyDescriptor family) throws AIOError;
          void deleteFamily(bytes table, bytes family) throws AIOError;
          void modifyFamily(bytes table, bytes familyName, AFamilyDescriptor familyDescriptor) throws AIOError;

          void enableTable(bytes table) throws AIOError;
          void disableTable(bytes table) throws AIOError;

          void flush(bytes table) throws AIOError;

          string getHBaseVersion() throws AIOError;
          AClusterStatus getClusterStatus() throws AIOError;
          array<ATableDescriptor> listTables() throws AIOError;

          boolean isTableEnabled(bytes table) throws AIOError;
          boolean tableExists(bytes table) throws AIOError;
          ATableDescriptor describeTable(bytes table) throws AIOError;

          AFamilyDescriptor describeFamily(bytes table, bytes family) throws AIOError;

          void put(bytes table, APut put) throws AIOError;

          AResult get(bytes table, AGet get) throws AIOError;

          int scannerOpen(bytes table, AScan scan) throws AIOError;
          void scannerClose(int scannerId) throws AIOError, AIllegalArgument;
          array<AResult> scannerGetRows(int scannerId, int numberOfRows) throws AIOError, AIllegalArgument;

          // TODO(hammer): New API discussion at HBASE-2609
          void delete(bytes table, ADelete delete) throws AIOError;

          Show
          Jeff Hammerbacher added a comment - Just to heartbeat here: I have completed the implementation of the methods listed below, which makes the server feature complete for my first cut (I've avoided the surgery commands, as it seems like a bad idea to let these commands be run from an Avro client. I've also avoided the atomic increment stuff, though that might make it into the patch). I'm going to do a refactoring pass some time next week. Does anyone have a strong interest in reviewing this code? Andrew? void createTable(ATableDescriptor table) throws AIOError, AIllegalArgument, ATableExists, AMasterNotRunning; void deleteTable(bytes table) throws AIOError; void modifyTable(bytes table, ATableDescriptor tableDescriptor) throws AIOError; void addFamily(bytes table, AFamilyDescriptor family) throws AIOError; void deleteFamily(bytes table, bytes family) throws AIOError; void modifyFamily(bytes table, bytes familyName, AFamilyDescriptor familyDescriptor) throws AIOError; void enableTable(bytes table) throws AIOError; void disableTable(bytes table) throws AIOError; void flush(bytes table) throws AIOError; string getHBaseVersion() throws AIOError; AClusterStatus getClusterStatus() throws AIOError; array<ATableDescriptor> listTables() throws AIOError; boolean isTableEnabled(bytes table) throws AIOError; boolean tableExists(bytes table) throws AIOError; ATableDescriptor describeTable(bytes table) throws AIOError; AFamilyDescriptor describeFamily(bytes table, bytes family) throws AIOError; void put(bytes table, APut put) throws AIOError; AResult get(bytes table, AGet get) throws AIOError; int scannerOpen(bytes table, AScan scan) throws AIOError; void scannerClose(int scannerId) throws AIOError, AIllegalArgument; array<AResult> scannerGetRows(int scannerId, int numberOfRows) throws AIOError, AIllegalArgument; // TODO(hammer): New API discussion at HBASE-2609 void delete(bytes table, ADelete delete) throws AIOError;
          Hide
          Jeff Hammerbacher added a comment -

          Oh also, should have mentioned: in addition to refactoring, I need to add tests.

          Show
          Jeff Hammerbacher added a comment - Oh also, should have mentioned: in addition to refactoring, I need to add tests.
          Hide
          Jeff Hammerbacher added a comment -

          added one more method:

          long incrementColumnValue(bytes table, bytes row, bytes family, bytes qualifier, long amount, boolean writeToWAL) throws AIOError;

          Show
          Jeff Hammerbacher added a comment - added one more method: long incrementColumnValue(bytes table, bytes row, bytes family, bytes qualifier, long amount, boolean writeToWAL) throws AIOError;
          Hide
          Andrew Purtell added a comment -

          Does anyone have a strong interest in reviewing this code? Andrew?

          Yes. Do you plan to submit to review.hbase.org ?

          Show
          Andrew Purtell added a comment - Does anyone have a strong interest in reviewing this code? Andrew? Yes. Do you plan to submit to review.hbase.org ?
          Hide
          Jeff Hammerbacher added a comment -

          Yes. Do you plan to submit to review.hbase.org ?

          Yep, thanks for volunteering. I'm flying home tomorrow and will refactor and add tests on the flight. I hope to have a review up by Thursday evening. Sorry for taking so long on this one, been traveling quite a bit!

          Show
          Jeff Hammerbacher added a comment - Yes. Do you plan to submit to review.hbase.org ? Yep, thanks for volunteering. I'm flying home tomorrow and will refactor and add tests on the flight. I hope to have a review up by Thursday evening. Sorry for taking so long on this one, been traveling quite a bit!
          Hide
          Jeff Hammerbacher added a comment -

          Here's an initial patch for this issue. I still have some javadoc and tests to write, but I wanted to get some eyeballs on the general approach before spending more time on each of those tasks. I'll stick up on Review Board right now.

          Show
          Jeff Hammerbacher added a comment - Here's an initial patch for this issue. I still have some javadoc and tests to write, but I wanted to get some eyeballs on the general approach before spending more time on each of those tasks. I'll stick up on Review Board right now.
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/
          -----------------------------------------------------------

          Review request for hbase.

          Summary
          -------

          Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA.

          This addresses bug HBASE-2400.

          Diffs


          trunk/bin/hbase 951826
          trunk/pom.xml 951826
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION

          Diff: http://review.hbase.org/r/128/diff

          Testing
          -------

          Thanks,

          Jeff

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/ ----------------------------------------------------------- Review request for hbase. Summary ------- Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA. This addresses bug HBASE-2400 . Diffs trunk/bin/hbase 951826 trunk/pom.xml 951826 trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION Diff: http://review.hbase.org/r/128/diff Testing ------- Thanks, Jeff
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@facebook.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review137
          -----------------------------------------------------------

          This looks great Hammer. No major comments.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment721>

          No. Setting a timestamp actually just sets the timerange with (stamp, stamp+1)

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment722>

          I've seen this in other places like HMaster where sometimes we initialize through command-line and other times programatically within something else, like in tests or to re-initialize... I think

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment723>

          This is one thing that is fairly mixed in the codebase. Not sure which is the right way, I like things spaced out but I might be in the minority.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment724>

          This is unfortunately mixed a bit... I think the client APIs say TimeStamp, not sure why exactly but I guess just align with that?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment725>

          The form I believe we follow is that inputs can be either null or 0-length array, but that data returned is a 0-length array. Would need to verify this is always the case but I think it is.

          • Jonathan
          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@facebook.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review137 ----------------------------------------------------------- This looks great Hammer. No major comments. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment721 > No. Setting a timestamp actually just sets the timerange with (stamp, stamp+1) trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment722 > I've seen this in other places like HMaster where sometimes we initialize through command-line and other times programatically within something else, like in tests or to re-initialize... I think trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment723 > This is one thing that is fairly mixed in the codebase. Not sure which is the right way, I like things spaced out but I might be in the minority. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment724 > This is unfortunately mixed a bit... I think the client APIs say TimeStamp, not sure why exactly but I guess just align with that? trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment725 > The form I believe we follow is that inputs can be either null or 0-length array, but that data returned is a 0-length array. Would need to verify this is always the case but I think it is. Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review138
          -----------------------------------------------------------

          Looks good Jeff. Some missing functionality IMHO. See my individual comments.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment726>

          That would be good.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment727>

          No split()?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java
          <http://review.hbase.org/r/128/#comment728>

          Export exists() to the client.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment729>

          Really appreciate this, which will help out with Avro encoding support in the REST interface. We should find a way for all of the connectors to share common Avro, Thrift, and Protobuf packages. I will refactor the protobuf stuff soon.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment730>

          Yes. And all of the connectors should drop such requests, throw exceptions, and warn upon similar events.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment731>

          Or just handle this generically as a string?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment732>

          Or just handle this generically as a string?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment733>

          0-length array.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment734>

          See comment on AvroServer in this regard.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment735>

          How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment736>

          Likewise, no support for user attributes here.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment737>

          No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string.

          No support for setBatch()

          Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment738>

          Missing split()

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment739>

          Missing attribute get and set, if not supporting read/write access via descriptor structs.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment740>

          Missing attribute get and set, if not supporting read/write access via descriptor structs.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment741>

          Missing exists()

          • Andrew
          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review138 ----------------------------------------------------------- Looks good Jeff. Some missing functionality IMHO. See my individual comments. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment726 > That would be good. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment727 > No split()? trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java < http://review.hbase.org/r/128/#comment728 > Export exists() to the client. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment729 > Really appreciate this, which will help out with Avro encoding support in the REST interface. We should find a way for all of the connectors to share common Avro, Thrift, and Protobuf packages. I will refactor the protobuf stuff soon. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment730 > Yes. And all of the connectors should drop such requests, throw exceptions, and warn upon similar events. trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment731 > Or just handle this generically as a string? trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment732 > Or just handle this generically as a string? trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment733 > 0-length array. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment734 > See comment on AvroServer in this regard. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment735 > How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment736 > Likewise, no support for user attributes here. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment737 > No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string. No support for setBatch() Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)? trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment738 > Missing split() trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment739 > Missing attribute get and set, if not supporting read/write access via descriptor structs. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment740 > Missing attribute get and set, if not supporting read/write access via descriptor structs. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment741 > Missing exists() Andrew
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review142
          -----------------------------------------------------------

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment743>

          Change to ascanToScan to be consistent with Put and Delete

          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java
          <http://review.hbase.org/r/128/#comment742>

          Change to resultsToAResults

          • Jeff
          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review142 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment743 > Change to ascanToScan to be consistent with Put and Delete trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java < http://review.hbase.org/r/128/#comment742 > Change to resultsToAResults Jeff
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 106

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line106>

          >

          > Likewise, no support for user attributes here.

          https://issues.apache.org/jira/browse/HBASE-2688

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 92

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line92>

          >

          > How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them.

          https://issues.apache.org/jira/browse/HBASE-2688

          I genuinely did not notice that these existed. I'll get a subsequent patch out once I clean this one up and it goes into trunk.

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 255

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line255>

          >

          > Missing attribute get and set, if not supporting read/write access via descriptor structs.

          https://issues.apache.org/jira/browse/HBASE-2688

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 248

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line248>

          >

          > Missing attribute get and set, if not supporting read/write access via descriptor structs.

          https://issues.apache.org/jira/browse/HBASE-2688

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 181

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line181>

          >

          > No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string.

          >

          > No support for setBatch()

          >

          > Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)?

          https://issues.apache.org/jira/browse/HBASE-2690 is for the caching and https://issues.apache.org/jira/browse/HBASE-2687 is for the filters. Thanks for letting me know how you did things in the REST interface; I'll try to mimic your approach. I held off on implementing because I wasn't quite sure what the API would look like.

          • Jeff

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review138
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 106 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line106 > > > Likewise, no support for user attributes here. https://issues.apache.org/jira/browse/HBASE-2688 On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 92 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line92 > > > How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them. https://issues.apache.org/jira/browse/HBASE-2688 I genuinely did not notice that these existed. I'll get a subsequent patch out once I clean this one up and it goes into trunk. On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 255 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line255 > > > Missing attribute get and set, if not supporting read/write access via descriptor structs. https://issues.apache.org/jira/browse/HBASE-2688 On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 248 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line248 > > > Missing attribute get and set, if not supporting read/write access via descriptor structs. https://issues.apache.org/jira/browse/HBASE-2688 On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 181 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line181 > > > No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string. > > No support for setBatch() > > Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)? https://issues.apache.org/jira/browse/HBASE-2690 is for the caching and https://issues.apache.org/jira/browse/HBASE-2687 is for the filters. Thanks for letting me know how you did things in the REST interface; I'll try to mimic your approach. I held off on implementing because I wasn't quite sure what the API would look like. Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review138 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 92

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line92>

          >

          > How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them.

          Jeff Hammerbacher wrote:

          https://issues.apache.org/jira/browse/HBASE-2688

          I genuinely did not notice that these existed. I'll get a subsequent patch out once I clean this one up and it goes into trunk.

          Sounds good. HTD and HCD can have attributes of arbitrary byte[].

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review138
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 92 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line92 > > > How do you deal with user attributes? A column can have an arbitrary set of them. Coprocessors will use this facility. Not necessary to support attribute access via fields of the descriptors if there are RPC methods available to read or write them. Jeff Hammerbacher wrote: https://issues.apache.org/jira/browse/HBASE-2688 I genuinely did not notice that these existed. I'll get a subsequent patch out once I clean this one up and it goes into trunk. Sounds good. HTD and HCD can have attributes of arbitrary byte[]. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review138 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 181

          > <http://review.hbase.org/r/128/diff/1/?file=979#file979line181>

          >

          > No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string.

          >

          > No support for setBatch()

          >

          > Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)?

          Jeff Hammerbacher wrote:

          https://issues.apache.org/jira/browse/HBASE-2690 is for the caching and https://issues.apache.org/jira/browse/HBASE-2687 is for the filters. Thanks for letting me know how you did things in the REST interface; I'll try to mimic your approach. I held off on implementing because I wasn't quite sure what the API would look like.

          Doesn't make sense with my current Scan API (super simple: table + number of rows). I filed https://issues.apache.org/jira/browse/HBASE-2705 to take care of this one at a later date.

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 386

          > <http://review.hbase.org/r/128/diff/1/?file=950#file950line386>

          >

          > Export exists() to the client.

          Done

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 335

          > <http://review.hbase.org/r/128/diff/1/?file=950#file950line335>

          >

          > No split()?

          Done

          On 2010-06-06 10:23:33, Andrew Purtell wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 324

          > <http://review.hbase.org/r/128/diff/1/?file=950#file950line324>

          >

          > That would be good.

          Removed comment; this will work with region names as well.

          • Jeff

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review138
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 181 > < http://review.hbase.org/r/128/diff/1/?file=979#file979line181 > > > No support for filters. For REST, the model classes recursively walk the filter structure and build a JSON representation that is then passed as a string. > > No support for setBatch() > > Consider support for setCacheBlocks() (regionserver level caching) and setCaching() (connector level caching)? Jeff Hammerbacher wrote: https://issues.apache.org/jira/browse/HBASE-2690 is for the caching and https://issues.apache.org/jira/browse/HBASE-2687 is for the filters. Thanks for letting me know how you did things in the REST interface; I'll try to mimic your approach. I held off on implementing because I wasn't quite sure what the API would look like. Doesn't make sense with my current Scan API (super simple: table + number of rows). I filed https://issues.apache.org/jira/browse/HBASE-2705 to take care of this one at a later date. On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 386 > < http://review.hbase.org/r/128/diff/1/?file=950#file950line386 > > > Export exists() to the client. Done On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 335 > < http://review.hbase.org/r/128/diff/1/?file=950#file950line335 > > > No split()? Done On 2010-06-06 10:23:33, Andrew Purtell wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java, line 324 > < http://review.hbase.org/r/128/diff/1/?file=950#file950line324 > > > That would be good. Removed comment; this will work with region names as well. Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review138 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/
          -----------------------------------------------------------

          (Updated 2010-06-08 20:05:01.662868)

          Review request for hbase.

          Changes
          -------

          Addressed all comments or filed tickets to address them at a later date.

          Summary
          -------

          Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA.

          This addresses bug HBASE-2400.

          Diffs (updated)


          trunk/bin/hbase 952782
          trunk/pom.xml 952782
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION

          Diff: http://review.hbase.org/r/128/diff

          Testing
          -------

          Thanks,

          Jeff

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/ ----------------------------------------------------------- (Updated 2010-06-08 20:05:01.662868) Review request for hbase. Changes ------- Addressed all comments or filed tickets to address them at a later date. Summary ------- Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA. This addresses bug HBASE-2400 . Diffs (updated) trunk/bin/hbase 952782 trunk/pom.xml 952782 trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION Diff: http://review.hbase.org/r/128/diff Testing ------- Thanks, Jeff
          Hide
          HBase Review Board added a comment -

          Message from: "Ryan Rawson" <ryanobjc@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review164
          -----------------------------------------------------------

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment806>

          you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment807>

          technically the serverName is the serverAddress + startCode... in the Java code is isnt fully exposed. Not sure what we want to do here, but this is probably fine as is.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment808>

          couldnt you use a empty string if there are no dead server names? im not sure if arrays can be 0 length in avro

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment809>

          same as deadServerNames.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment810>

          presumably in the future we would store the authoritative list in avro and Java would use that enum. Has anyone done this pattern yet?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment811>

          the compression can never be null, because the "NONE" is the catch all here.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment812>

          do we need to make these fields nullable? usually they are true/false in the java code.

          Is this some semi-mechanical translation from a java api?

          • Ryan
          Show
          HBase Review Board added a comment - Message from: "Ryan Rawson" <ryanobjc@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review164 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment806 > you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment807 > technically the serverName is the serverAddress + startCode... in the Java code is isnt fully exposed. Not sure what we want to do here, but this is probably fine as is. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment808 > couldnt you use a empty string if there are no dead server names? im not sure if arrays can be 0 length in avro trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment809 > same as deadServerNames. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment810 > presumably in the future we would store the authoritative list in avro and Java would use that enum. Has anyone done this pattern yet? trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment811 > the compression can never be null, because the "NONE" is the catch all here. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment812 > do we need to make these fields nullable? usually they are true/false in the java code. Is this some semi-mechanical translation from a java api? Ryan
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 111

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line111>

          >

          > do we need to make these fields nullable? usually they are true/false in the java code.

          >

          > Is this some semi-mechanical translation from a java api?

          I use the same Avro record for table creation and modification as well as description. For create table, I want the fields to be nullable because the user should not have to specify a value.

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 94

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line94>

          >

          > the compression can never be null, because the "NONE" is the catch all here.

          Same as below: I use the same record for family creation, modification, and description. Avro currently doesn't have default values on write, so making this field nullable means we can do smart things if the user doesn't specify a compression algorithm during Family creation.

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 78

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line78>

          >

          > same as deadServerNames.

          Yeah I should make these 0-length arrays.

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 73

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line73>

          >

          > couldnt you use a empty string if there are no dead server names? im not sure if arrays can be 0 length in avro

          Will make a 0-length array

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 66

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line66>

          >

          > technically the serverName is the serverAddress + startCode... in the Java code is isnt fully exposed. Not sure what we want to do here, but this is probably fine as is.

          Yeah since Avro records don't have methods, you can think of this field as a materialization of the Java logic.

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 34

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line34>

          >

          > you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world.

          Let me know what you want me to do here. I was just copying the fields directly from the Java objects.

          • Jeff

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review164
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 111 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line111 > > > do we need to make these fields nullable? usually they are true/false in the java code. > > Is this some semi-mechanical translation from a java api? I use the same Avro record for table creation and modification as well as description. For create table, I want the fields to be nullable because the user should not have to specify a value. On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 94 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line94 > > > the compression can never be null, because the "NONE" is the catch all here. Same as below: I use the same record for family creation, modification, and description. Avro currently doesn't have default values on write, so making this field nullable means we can do smart things if the user doesn't specify a compression algorithm during Family creation. On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 78 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line78 > > > same as deadServerNames. Yeah I should make these 0-length arrays. On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 73 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line73 > > > couldnt you use a empty string if there are no dead server names? im not sure if arrays can be 0 length in avro Will make a 0-length array On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 66 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line66 > > > technically the serverName is the serverAddress + startCode... in the Java code is isnt fully exposed. Not sure what we want to do here, but this is probably fine as is. Yeah since Avro records don't have methods, you can think of this field as a materialization of the Java logic. On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 34 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line34 > > > you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world. Let me know what you want me to do here. I was just copying the fields directly from the Java objects. Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review164 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Ryan Rawson" <ryanobjc@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review168
          -----------------------------------------------------------

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment824>

          in this case we have to distinguish between 'give me family X' and 'give me family X 0 length qualifier' which are in fact different queries and are both representable in the standard Get Java API.

          the java code does this by using a map of a map in the Get object:
          Map<byte[], Set<byte[]>> familyMap;
          where the key is the family, and the value is the set of qualifiers for said family. If you want to get a family the code will use 'null' as the Set value.

          For the Avro API we don't have to do it in the same way, but we need to know the difference between those queries. perhaps using AColumn 'family = foo, qualifier=null' can be the 'give me the family' and 'family = foo, qualifier = 0 length bytes' can be the other?

          • Ryan
          Show
          HBase Review Board added a comment - Message from: "Ryan Rawson" <ryanobjc@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review168 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment824 > in this case we have to distinguish between 'give me family X' and 'give me family X 0 length qualifier' which are in fact different queries and are both representable in the standard Get Java API. the java code does this by using a map of a map in the Get object: Map<byte[], Set<byte[]>> familyMap; where the key is the family, and the value is the set of qualifiers for said family. If you want to get a family the code will use 'null' as the Set value. For the Avro API we don't have to do it in the same way, but we need to know the difference between those queries. perhaps using AColumn 'family = foo, qualifier=null' can be the 'give me the family' and 'family = foo, qualifier = 0 length bytes' can be the other? Ryan
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          On 2010-06-09 17:54:20, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 34

          > <http://review.hbase.org/r/128/diff/2/?file=1157#file1157line34>

          >

          > you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world.

          Jeff Hammerbacher wrote:

          Let me know what you want me to do here. I was just copying the fields directly from the Java objects.

          After talking in IRC, it sounds like we want to get rid of this guy. Done.

          • Jeff

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review164
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> On 2010-06-09 17:54:20, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 34 > < http://review.hbase.org/r/128/diff/2/?file=1157#file1157line34 > > > you can probably just use 'hostname' and 'port'. There was a recent patch in trunk that is attempting to get rid of IP addresses (they cause issues when they dont align with DNS names, etc) and generally move us to a DNS name world. Jeff Hammerbacher wrote: Let me know what you want me to do here. I was just copying the fields directly from the Java objects. After talking in IRC, it sounds like we want to get rid of this guy. Done. Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review164 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/
          -----------------------------------------------------------

          (Updated 2010-06-09 18:22:12.245370)

          Review request for hbase.

          Changes
          -------

          Addressed Ryan's requests

          Summary
          -------

          Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA.

          This addresses bug HBASE-2400.

          Diffs (updated)


          trunk/bin/hbase 953193
          trunk/pom.xml 953193
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION

          Diff: http://review.hbase.org/r/128/diff

          Testing
          -------

          Thanks,

          Jeff

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/ ----------------------------------------------------------- (Updated 2010-06-09 18:22:12.245370) Review request for hbase. Changes ------- Addressed Ryan's requests Summary ------- Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA. This addresses bug HBASE-2400 . Diffs (updated) trunk/bin/hbase 953193 trunk/pom.xml 953193 trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION Diff: http://review.hbase.org/r/128/diff Testing ------- Thanks, Jeff
          Hide
          HBase Review Board added a comment -

          Message from: "Ryan Rawson" <ryanobjc@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review171
          -----------------------------------------------------------

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr
          <http://review.hbase.org/r/128/#comment832>

          if this is generated from the genavro, is it possible to get a maven rule to generate this? Or is that not ready yet?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment827>

          does it make sense to reuse AColumn here?

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment828>

          The Java API gets its speed by essentially taking a Result which is an array of KeyValue, which are just byte arrays and serializing it all as one large array. On the client side, the client reads the entire array then builds the KeyValues that provide a view onto this one array.

          I don't know how this performance improvement could be done in this avro interface, but I thought I'd bring it up for reference.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment829>

          it would be nice to collapse AResultEntry and AColumnValue, they seem to be almost the same thing.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment830>

          technically speaking getRowOrBefore() isnt a 'public' method, it is supposed to be mostly used for META support, and I think we are trending to 'dont use for general purpose'.

          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro
          <http://review.hbase.org/r/128/#comment831>

          these apis are good, but i'm wondering if you'd be open to a new experimental scanner API we have been interested in for the base RPC...

          essentially right now you need 3 RPC calls even to retrieve a small amount of data. What would an API look like that lets you open, get rows and have implicit closes if you hit the end of the scan in your 'number of records' parameter? We'd still have explicit closes for premature client-driven scan-terminations, but if your goal is to scan to the end, then why do an explicit close? Also why not have the 'open' also start to return data? The returned value would probably have to be a struct..

          This is more of an optional exercise, so if you dont feel the need, it's fine.

          • Ryan
          Show
          HBase Review Board added a comment - Message from: "Ryan Rawson" <ryanobjc@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review171 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr < http://review.hbase.org/r/128/#comment832 > if this is generated from the genavro, is it possible to get a maven rule to generate this? Or is that not ready yet? trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment827 > does it make sense to reuse AColumn here? trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment828 > The Java API gets its speed by essentially taking a Result which is an array of KeyValue, which are just byte arrays and serializing it all as one large array. On the client side, the client reads the entire array then builds the KeyValues that provide a view onto this one array. I don't know how this performance improvement could be done in this avro interface, but I thought I'd bring it up for reference. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment829 > it would be nice to collapse AResultEntry and AColumnValue, they seem to be almost the same thing. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment830 > technically speaking getRowOrBefore() isnt a 'public' method, it is supposed to be mostly used for META support, and I think we are trending to 'dont use for general purpose'. trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro < http://review.hbase.org/r/128/#comment831 > these apis are good, but i'm wondering if you'd be open to a new experimental scanner API we have been interested in for the base RPC... essentially right now you need 3 RPC calls even to retrieve a small amount of data. What would an API look like that lets you open, get rows and have implicit closes if you hit the end of the scan in your 'number of records' parameter? We'd still have explicit closes for premature client-driven scan-terminations, but if your goal is to scan to the end, then why do an explicit close? Also why not have the 'open' also start to return data? The returned value would probably have to be a struct.. This is more of an optional exercise, so if you dont feel the need, it's fine. Ryan
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          On 2010-06-09 19:24:25, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr, line 1

          > <http://review.hbase.org/r/128/diff/3/?file=1190#file1190line1>

          >

          > if this is generated from the genavro, is it possible to get a maven rule to generate this? Or is that not ready yet?

          Yes, this should definitely be done during the build. See https://issues.apache.org/jira/browse/AVRO-572.

          On 2010-06-09 19:24:25, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 155

          > <http://review.hbase.org/r/128/diff/3/?file=1191#file1191line155>

          >

          > The Java API gets its speed by essentially taking a Result which is an array of KeyValue, which are just byte arrays and serializing it all as one large array. On the client side, the client reads the entire array then builds the KeyValues that provide a view onto this one array.

          >

          > I don't know how this performance improvement could be done in this avro interface, but I thought I'd bring it up for reference.

          My comment here is not for performance considerations, it's for concision and related to your previous comment (on line 140): AColumn, AResultEntry, and AColumnValue all do approximately the same thing. I could make the fields nullable and use one Avro record for each. Pro: I have less generated classes. Con: the generated class I have is less task-specific. To be honest, since there are not a lot of Avro services out there, it's hard to say which is the best practice. I'm happy to take feedback but decided that being more verbose with my number of objects was better.

          On 2010-06-09 19:24:25, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 156

          > <http://review.hbase.org/r/128/diff/3/?file=1191#file1191line156>

          >

          > it would be nice to collapse AResultEntry and AColumnValue, they seem to be almost the same thing.

          (see above comment)

          On 2010-06-09 19:24:25, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 268

          > <http://review.hbase.org/r/128/diff/3/?file=1191#file1191line268>

          >

          > these apis are good, but i'm wondering if you'd be open to a new experimental scanner API we have been interested in for the base RPC...

          >

          > essentially right now you need 3 RPC calls even to retrieve a small amount of data. What would an API look like that lets you open, get rows and have implicit closes if you hit the end of the scan in your 'number of records' parameter? We'd still have explicit closes for premature client-driven scan-terminations, but if your goal is to scan to the end, then why do an explicit close? Also why not have the 'open' also start to return data? The returned value would probably have to be a struct..

          >

          > This is more of an optional exercise, so if you dont feel the need, it's fine.

          Yeah that would be nice; you could return (int scannerId, bytes[] row, resultScanner result). In the Python client, I don't expose open/close; the Python clients just scan.

          On 2010-06-09 19:24:25, Ryan Rawson wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 230

          > <http://review.hbase.org/r/128/diff/3/?file=1191#file1191line230>

          >

          > technically speaking getRowOrBefore() isnt a 'public' method, it is supposed to be mostly used for META support, and I think we are trending to 'dont use for general purpose'.

          Noted. I will remove the comment.

          • Jeff

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review171
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> On 2010-06-09 19:24:25, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr, line 1 > < http://review.hbase.org/r/128/diff/3/?file=1190#file1190line1 > > > if this is generated from the genavro, is it possible to get a maven rule to generate this? Or is that not ready yet? Yes, this should definitely be done during the build. See https://issues.apache.org/jira/browse/AVRO-572 . On 2010-06-09 19:24:25, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 155 > < http://review.hbase.org/r/128/diff/3/?file=1191#file1191line155 > > > The Java API gets its speed by essentially taking a Result which is an array of KeyValue, which are just byte arrays and serializing it all as one large array. On the client side, the client reads the entire array then builds the KeyValues that provide a view onto this one array. > > I don't know how this performance improvement could be done in this avro interface, but I thought I'd bring it up for reference. My comment here is not for performance considerations, it's for concision and related to your previous comment (on line 140): AColumn, AResultEntry, and AColumnValue all do approximately the same thing. I could make the fields nullable and use one Avro record for each. Pro: I have less generated classes. Con: the generated class I have is less task-specific. To be honest, since there are not a lot of Avro services out there, it's hard to say which is the best practice. I'm happy to take feedback but decided that being more verbose with my number of objects was better. On 2010-06-09 19:24:25, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 156 > < http://review.hbase.org/r/128/diff/3/?file=1191#file1191line156 > > > it would be nice to collapse AResultEntry and AColumnValue, they seem to be almost the same thing. (see above comment) On 2010-06-09 19:24:25, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 268 > < http://review.hbase.org/r/128/diff/3/?file=1191#file1191line268 > > > these apis are good, but i'm wondering if you'd be open to a new experimental scanner API we have been interested in for the base RPC... > > essentially right now you need 3 RPC calls even to retrieve a small amount of data. What would an API look like that lets you open, get rows and have implicit closes if you hit the end of the scan in your 'number of records' parameter? We'd still have explicit closes for premature client-driven scan-terminations, but if your goal is to scan to the end, then why do an explicit close? Also why not have the 'open' also start to return data? The returned value would probably have to be a struct.. > > This is more of an optional exercise, so if you dont feel the need, it's fine. Yeah that would be nice; you could return (int scannerId, bytes[] row, resultScanner result). In the Python client, I don't expose open/close; the Python clients just scan. On 2010-06-09 19:24:25, Ryan Rawson wrote: > trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro, line 230 > < http://review.hbase.org/r/128/diff/3/?file=1191#file1191line230 > > > technically speaking getRowOrBefore() isnt a 'public' method, it is supposed to be mostly used for META support, and I think we are trending to 'dont use for general purpose'. Noted. I will remove the comment. Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review171 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/
          -----------------------------------------------------------

          (Updated 2010-06-11 22:56:49.438699)

          Review request for hbase.

          Changes
          -------

          Hey, I've added more tests and addressed all of the review comments. Any chance we can get this into trunk? I'd like to start addressing some of the follow-on issues.

          Summary
          -------

          Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA.

          This addresses bug HBASE-2400.
          http://issues.apache.org/jira/browse/HBASE-2400

          Diffs (updated)


          trunk/bin/hbase 953940
          trunk/pom.xml 953940
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION
          trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION

          Diff: http://review.hbase.org/r/128/diff

          Testing
          -------

          Thanks,

          Jeff

          Show
          HBase Review Board added a comment - Message from: "Jeff Hammerbacher" <jeff.hammerbacher@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/ ----------------------------------------------------------- (Updated 2010-06-11 22:56:49.438699) Review request for hbase. Changes ------- Hey, I've added more tests and addressed all of the review comments. Any chance we can get this into trunk? I'd like to start addressing some of the follow-on issues. Summary ------- Initial patch; some javadoc and tests missing, but I wanted to get some initial feedback on the approach. My apologies for sticking a patch on the JIRA before the review. I should have read further on the HowToContribute JIRA. This addresses bug HBASE-2400 . http://issues.apache.org/jira/browse/HBASE-2400 Diffs (updated) trunk/bin/hbase 953940 trunk/pom.xml 953940 trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/AvroUtil.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AAlreadyExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AClusterStatus.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumn.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AColumnValue.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ACompressionAlgorithm.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ADelete.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AFamilyDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AGet.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AIllegalArgument.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AMasterNotRunning.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/APut.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ARegionLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResult.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AResultEntry.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AScan.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerAddress.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerInfo.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/AServerLoad.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableDescriptor.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATableExists.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/ATimeRange.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/HBase.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/IOError.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/generated/TCell.java PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.avpr PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/hbase.genavro PRE-CREATION trunk/src/main/java/org/apache/hadoop/hbase/avro/package.html PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/avro/TestAvroServer.java PRE-CREATION Diff: http://review.hbase.org/r/128/diff Testing ------- Thanks, Jeff
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/128/#review209
          -----------------------------------------------------------

          Ship it!

          • Andrew
          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/128/#review209 ----------------------------------------------------------- Ship it! Andrew
          Hide
          ryan rawson added a comment -

          I just committed v3 of the patch from reviewboard.

          Show
          ryan rawson added a comment - I just committed v3 of the patch from reviewboard.

            People

            • Assignee:
              Jeff Hammerbacher
              Reporter:
              Andrew Purtell
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development