> On Oct. 8, 2012, 5:44 a.m., Denny Ye wrote: > > I reviewed code again and found another advice. 1. Http Source is kind of > > special source, not original one. Thus, it should be migrated into > > 'flume-ng-sources' folder. It's recommended. 2. Pay attention to the socket > > stream from HttpServletRequest.getReader() or > > HttpServletRequest.getInputStream(). If there is no any catch for failure > > about those streams in custom HTTPSourceDeserializer, that might be serious > > leak of file descriptor or connection. HTTPSource should close stream > > assuringly.
1. I am open to refactoring it, but I think we could do that later. For now, I'd rather leave it as is. 2. No, the servlet request's streams need not be closed by the servlet. That is handled by the container. Closing the streams will interfere with lifecycle of the servlet. This is not an authoritarian source but read this: http://stackoverflow.com/questions/1808248/is-is-necessary-to-close-the-input-stream-returned-from-httpservletrequest. Since I did not create the stream, I should not close it either. Also - take a look at this: http://docstore.mik.ua/orelly/java-ent/jenut/ch05_02.htm - Example 5.2 shows a servlet throwing an exception - the streams are not closed, because the container is expected to handle this. So there is no reason to worry about it. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7465/#review12220 ----------------------------------------------------------- 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 > >
