> On Oct. 9, 2012, 7:23 p.m., Mike Percy wrote: > > Looking pretty good. A few suggestions: > > 1. I think we should add the capability to serialize a response somehow. > > Right now it just returns a blank text/html document on success. > > Serializing an arbitrary response could allow us to implement HTTP-based > > RPC protocols using this as a basis. > > 2. Right now we bind everything to /* instead of a specific endpoint URL > > like / or something else. It's best to use a single endpoint to allow for > > extension later, such as allowing different deserialization handlers per > > URL. > > 3. The deserialization we would do here has logic that we could use in a > > file-reading client built on top of FLUME-1425. It would be nice to create > > a deserialization interface that takes an InputStream to share between > > these use cases if possible, so we can have one set of plugins that could > > potentially work with both. > > 4. Nit: I agree with Denny that this should probably live in > > flume-ng-sources to avoid increasing the size of flume-ng-core
Thanks for the review, Mike! 1 and 3 sort of conflict with each other. So I have implemented 1 and not 3. Agreed with 2. I am not inclined to make this a separate module - since the point of moving to a separate module is usually because of the additional dependencies the new code would pull in, or for ease of distribution. I don't see either here. So will submit a patch with 1 and 2 done. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7465/#review12285 ----------------------------------------------------------- On Oct. 8, 2012, 5:30 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7465/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2012, 5:30 a.m.) > > > Review request for Flume. > > > Description > ------- > > Added an HTTP source and a reference deserializer to deserialize events from > json format. > > > This addresses bug FLUME-1199. > https://issues.apache.org/jira/browse/FLUME-1199 > > > Diffs > ----- > > flume-ng-core/pom.xml 4592a9d > flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceDeserializer.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/http/JSONDeserializer.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/http/FlumeHttpServletRequestWrapper.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/http/TestJSONDeserializer.java > PRE-CREATION > flume-ng-doc/sphinx/FlumeUserGuide.rst 953a670 > pom.xml e19d2d2 > > Diff: https://reviews.apache.org/r/7465/diff/ > > > Testing > ------- > > Added several unit tests. > > > Thanks, > > Hari Shreedharan > >
