----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16650/#review32853 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/16650/#comment62433> There's a great deal of non-semantic changes in this patch. Imports are re-ordered or turned into .*. Comments are reflowed. It appears that you've left your IDE's standard Java rules on and blindly accepted all the changes it made. I don't particularly want to make you revert them all but it makes the patch confusing to read and will cause issues to people who need to manually merge - because they will have a great deal of changes to ensure are applied correctly and many of them not semantic. If we let people apply non semantic changes made by their IDEs then our change logs will be full of changes from warring IDEs. flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/16650/#comment62428> I think that changes for SyslogParser should be in a separate patch with a separate Jira issue. The syslog change has nothing to do with elasticsearch and will confuse people. flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchIndexBuilder.java <https://reviews.apache.org/r/16650/#comment62431> So why the name change of an existing interface? As far as I can tell this is identical to the previous ElasticSearchIndexRequestBuilderFactory. Users who currently have a custom implementation of this interface are going to be broken by this change. flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java <https://reviews.apache.org/r/16650/#comment62436> You need to keep backwards compatibility here. flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSinkConstants.java <https://reviews.apache.org/r/16650/#comment62437> Spelling of DEFAULT flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java <https://reviews.apache.org/r/16650/#comment62444> Why am I not allowed to override the port here? flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java <https://reviews.apache.org/r/16650/#comment62438> I don't think we should force the content to UTF-8 by default. It would be quite surprising to developers who may have carefully crafted the event they store somewhere earlier in the pipeline. In fact, how does this interact with HTML encoding and events in other character sets? There should be unit tests around this point. flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java <https://reviews.apache.org/r/16650/#comment62440> I would say "Sending bulk request to elasticsearch cluster" Bulk is not a noun in this context. flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchTransportClient.java <https://reviews.apache.org/r/16650/#comment62441> Same grammar comment as before. flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java <https://reviews.apache.org/r/16650/#comment62442> This test needs to be re-included and pass. pom.xml <https://reviews.apache.org/r/16650/#comment61851> Why only 14.0.1 when 16 has just come out? - Edward Sargisson On Jan. 7, 2014, 9:59 a.m., Pawe? wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16650/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2014, 9:59 a.m.) > > > Review request for Flume, Brock Noland, Hari Shreedharan, and Mike Percy. > > > Repository: flume-git > > > Description > ------- > > The patch contains changes in elasticsearch sink. It gives possibility to > swich between Transport Client and REST Client to elasticsearch. > Jest library (https://github.com/searchbox-io/Jest) client was used to > communicate with elasticsearch by HTTP API. It uses guava 14 so it was > necessary also to change version of guava used in flume (11.0.2 -> 14.0.1) > > Patch FLUME-2225-0.patch > > > Diffs > ----- > > flume-ng-core/pom.xml e41bcc2 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java > 557d121 > flume-ng-sinks/flume-ng-elasticsearch-sink/pom.xml bdc21d1 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchIndexRequestBuilderFactory.java > 6effe34 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchIndexBuilder.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchIndexRequestBuilderFactory.java > 8e77a1e > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java > e38ab19 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSinkConstants.java > dd0c59d > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/EventSerializerIndexRequestBuilderFactory.java > c71b2e5 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/IndexNameBuilder.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/TimeBasedIndexNameBuilder.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/TimestampedEvent.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClient.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClientFactory.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchTransportClient.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java > 48eafdf > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchDynamicSerializer.java > 43a4b12 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchIndexRequestBuilderFactory.java > 1e4e119 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchLogStashEventSerializer.java > 9dff4b0 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java > 71789e8 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TimeBasedIndexNameBuilderTest.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TimestampedEventTest.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchClientFactory.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchRestClient.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchTransportClient.java > PRE-CREATION > pom.xml 3c741c3 > > Diff: https://reviews.apache.org/r/16650/diff/ > > > Testing > ------- > > > Thanks, > > Pawe? > >
