> On Aug. 3, 2014, 4:42 p.m., Pawel wrote: > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClientFactory.java, > > line 79 > > <https://reviews.apache.org/r/24213/diff/1/?file=649255#file649255line79> > > > > Isn't this way to instantiate a class less readable? I think when i > > future you want to search for class usage you want find it with any tool > > because of instantiating by class name. You want to be able to instantiate > > custom client that user can pass in configuration > > "com.blablabla.NEW_CLIENT_TYPE? ? > > Bastian Germann wrote: > Right, that is the new feature provided by this changeset. It is less > readable, but it is required for that feature. > > Pawel wrote: > Oh sorry, I didn't read the issue title. BTW it is OK for me to have > ability to have custom client types but why also existing clients are > instantiated by name? Can't they be constructed by "new" operator and in > other cases there will be instantiating by class name? Only asking what is a > benefit of this solution over this below: > > if (rest) { > new rest > } else if (transport) > new transport > } else { > instantiate by custom name > }
That would be fine. There is no benefit in my version. - Bastian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24213/#review49429 ----------------------------------------------------------- On Aug. 3, 2014, 2:22 p.m., Bastian Germann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24213/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2014, 2:22 p.m.) > > > Review request for Flume. > > > Repository: flume-git > > > Description > ------- > > (from JIRA:) > This Patch changes the configuration of the ElasticSearchClient and also the > ElasticSearchSink. Some parameters that are only relevant for the client are > directly passed to it without using the ElasticSearchClientFactory in > between. The affected tests are changed. > The new feature comes with ElasticSearchClientFactory. It is extended to > create instances of arbitrary FQCNs additionally to rest and transport > clients. There is also a test case for that feature. > Also the way a local transport client for testing is created changed to only > affect the client, but not the sink or the client factory. > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java > 1d9dfce > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClient.java > 655e00a > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClientFactory.java > 873157a > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java > 0d1c37f > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchTransportClient.java > d44c8ad > > 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/TestElasticSearchIndexRequestBuilderFactory.java > 8022111 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java > 15546c1 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TimeBasedIndexNameBuilderTest.java > 678342a > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchClientFactory.java > 4b70b65 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchRestClient.java > b7d8822 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/client/TestElasticSearchTransportClient.java > b7b8e74 > > Diff: https://reviews.apache.org/r/24213/diff/ > > > Testing > ------- > > > Thanks, > > Bastian Germann > >
