----------------------------------------------------------- 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 On 2010-06-05 23:16:10, Jeff Hammerbacher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/128/ > ----------------------------------------------------------- > > (Updated 2010-06-05 23:16:10) > > > 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 > >
