[jira] Commented: (HBASE-1400) Improve REST interface semantics and efficiency

["Brian Beggs (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12715251#action_12715251 ] 

Brian Beggs commented on HBASE-1400:
------------------------------------

If I had the opportunity to go back and write the HBase REST interface now, after knowing a lot more about Hbase and having used Jersey on another project, this is how I would want to write the interface.

What I like:

- Having Jersey integrated with the project takes away so much manual work that was being done before with the Response messages and input parsing.  That alone will make this implementation so much less error prone that what is currently in the interface.  I spent a lot of time chasing down I/O defects in the current implementation which left me less time to focus on functionality.

- Generally everything about this implementation is much cleaner than prior implementations.
 
What I would like to see:

- I do think it would be beneficial to have JSON support since the current interface supports this.  I would be willing to contribute this portion.

Thoughts:

- I think that having JSON I/O is still beneficial to the interface.  While the addition of protobufs makes for an attractive option to JSON, there may still be a need for a JSON interface for something such as JavaScript (Only Firefox is officially supported for JS protobufs).

- As with the prior implementation there is some redundant code centering around parsing/serializing of I/O.  For example in the TableResources class there are 3 methods to serialize output (text, xml and protobufs).  I had tried to centralize alot of this serialization code into it's own class using the visitor pattern for the last iteration to keep the implementation classes a little cleaner, reduce code redundancy and to make it a bit easier to extend the interface to add new I/O types.  While the old interface had redundancy in the fact that there was serialization or parsing for each data type, I feel that the I/O is wrapped a bit too tightly in this current implementation and more closely resembles the REST implementation before I made the I/O more modular.  

I think that there is a bit of redundancy in the implementation as it stands currently, and I also think that it can be factored out relatively easily... (see below)

- I think it would be desirable to add some model classes to the interface that could be used for I/O parsing/Serialization.  These model classes could use the JAX-B XML and JSON parsing/serialization that is already available with Jersey.  This would make the code more modular and easier to extend.  Then to parse the protobuf (or if the JAX-B implementations are not fast enough) adding a provider to handle the I/O is relatively easy.  Take a look at this article:
http://www.javarants.com/2008/12/27/using-jax-rs-with-protocol-buffers-for-high-performance-rest-apis/

Google says that protobufs generated classes should not be treated as first class objects, and while I don't feel that this implementation necessary treats them as such.  I think there would be benefit from wrapping the protobufs into the model classes discussed above and accessing them this way.  

Taking all of the above into consideration expanding the REST interface becomes much easier.

For example:
Lets say we want to add an additional piece of information into the list of tables that gets returned when you call the root of the webapp @Path("/")

- Current implementation:
{code:title=TableResource.java (partial)|borderStyle=solid}
  @GET
  @Produces(MIMETYPE_TEXT)
  public Response getAsText(@Context UriInfo uriInfo) {
    if (LOG.isDebugEnabled()) {
      LOG.debug("GET " + uriInfo.getAbsolutePath() + " as " + MIMETYPE_TEXT);
    }
    try {
      RESTServer.getInstance().serviceRequests++;
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.INTERNAL_SERVER_ERROR);
    }
    try {
      StringWriter writer = new StringWriter();
      for (HTableDescriptor htd: getTableList()) {
        if (htd.isMetaRegion()) {
          continue;
        }
        writer.append(htd.getNameAsString());
        writer.append('\n');
      }
      ResponseBuilder response = Response.ok(writer.toString());
      response.cacheControl(cacheControl);
      return response.build();
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.SERVICE_UNAVAILABLE);
    }
  }

  @GET
  @Produces(MIMETYPE_XML)
  public Response getAsXML(@Context UriInfo uriInfo) {
    if (LOG.isDebugEnabled()) {
      LOG.debug("GET " + uriInfo.getAbsolutePath() + " as " + MIMETYPE_XML);
    }
    try {
      RESTServer.getInstance().serviceRequests++;
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.INTERNAL_SERVER_ERROR);
    }
    try {
      StringWriter writer = new StringWriter();
      XMLOutputter result = new XMLOutputter(writer, "US-ASCII");
      result.startTag("TableList");
      for (HTableDescriptor htd: getTableList()) {
        if (htd.isMetaRegion()) {
          continue;
        }
        result.startTag("table");
        result.attribute("name", htd.getNameAsString());
        result.endTag();
      }
      result.endTag();
      result.endDocument();
      ResponseBuilder response = Response.ok(writer.toString());
      response.cacheControl(cacheControl);
      return response.build();
    } catch (UnsupportedEncodingException e) {
      throw new WebApplicationException(e, 
                  Response.Status.INTERNAL_SERVER_ERROR);
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.SERVICE_UNAVAILABLE);
    }
  }

  @GET
  @Produces(MIMETYPE_PROTOBUF)
  public Response getAsProtobuf(@Context UriInfo uriInfo) {
    if (LOG.isDebugEnabled()) {
      LOG.debug("GET " + uriInfo.getAbsolutePath() + " as " +
        MIMETYPE_PROTOBUF);
    }
    try {
      RESTServer.getInstance().serviceRequests++;
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.INTERNAL_SERVER_ERROR);
    }
    try {
      TableList.Builder list = TableList.newBuilder();
      for (HTableDescriptor htd: getTableList()) {
        if (htd.isMetaRegion()) {
          continue;
        }
        list.addName(htd.getNameAsString());
      }
      ResponseBuilder response = Response.ok(list.build().toByteArray());
      response.cacheControl(cacheControl);
      return response.build();
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.SERVICE_UNAVAILABLE);
    }
  }
{code}

To modify the code change the following:
-* Modify each method that serializes the output in the TableResource class (currently 3, but +1 if we add JSON).
-* recreate the protobuf and recompile.  (then make sure that this doesn't effect any of the other classes that use this file)

So this is at least 4 methods you have to touch + n methods that may be effected by the protobuf change.

- Modified implementation:

{code:title=TableResource.java (partial)|borderStyle=solid}
  @GET
  @Produces(MIMETYPE_TEXT, MIMETYPE_XML, MIMETYPE_PROTOBUF)
  public TableModel get(@Context UriInfo uriInfo) {
    List<TableModel> tables = new ArrayList<TableModel>();
    if (LOG.isDebugEnabled()) {
      LOG.debug("GET " + uriInfo.getAbsolutePath() + " as " + MIMETYPE_TEXT);
    }
    try {
      RESTServer.getInstance().serviceRequests++;
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.INTERNAL_SERVER_ERROR);
    }
    try {
      for (HTableDescriptor htd: getTableList()) {
        if (htd.isMetaRegion()) {
          continue;
        }
        tables.add(new TableModel(htd.getName()));
      }
      return models;
    } catch (IOException e) {
      throw new WebApplicationException(e, 
                  Response.Status.SERVICE_UNAVAILABLE);
    }
  }
{code}

{code:title=TableModel.java (partial)|borderStyle=solid}
@XmlRootElement(name="table")
@XmlType(name = "", propOrder = {"name"})

public class TableModel implements Serializable {

	private static final long serialVersionUID = 2871762412753722057L;
	private String name;
	
	/**
	 * @param name
	 */
	public TableModel(String name) {
		super();
		this.name = name;
	}
	
	public TableModel() {}

	/**
	 * @return the name
	 */
	public String getName() {
		return name;
	}

	/**
	 * @param name the name to set
	 */
	public void setName(String name) {
		this.name = name;
	}
}
{code}
To modify the code change the following:
-* Add the field into the model object with the annotation
-* modify the 1 method that does the work to get the list of tables
-* make sure that the modified protobuf class doesn't effect the 1 class that wraps it.

This is 3 methods that need to be modified.

I think that a modifying the implementation using these ideas makes for a cleaner easier to maintain interface.  

I would like to contribute back to this and will see if I can get some time on my schedule for it.

@Stack {quote}Andrew: You've been busy. This looks great. How much does it differ from current REST (I've not done a side-by-side but they seem close){quote}
No the interfaces are not the same, but I think that if you guys want to have a functional, easier to maintain REST implementation I think that the semantics of this system are much better than what is available currently.

Overall I think that it's good work and would ultimately be a great contribution to the project.

And that's my $3.50.  Let me know if you guys would like any clarification.


> Improve REST interface semantics and efficiency
> -----------------------------------------------
>
>                 Key: HBASE-1400
>                 URL: https://issues.apache.org/jira/browse/HBASE-1400
>             Project: Hadoop HBase
>          Issue Type: Improvement
>          Components: rest
>            Reporter: Andrew Purtell
>            Priority: Minor
>         Attachments: stargate-0.0.1.zip, stargate-testing.pdf, stargate.pdf
>
>
> Improve the semantics of the REST interface: more metadata operations, bulk updates, protobufs (if Accept equals "application/x-protobuf" for GET or Content-Type equals the same for PUT or POST) instead of multipart/related (which is not supported now anyway) etc. for general efficiency and support for queries or scanners that return multiple KeyValues. 
> I am working on a proposal.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.