----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33297/#review82358 -----------------------------------------------------------
samza-elasticsearch/src/main/java/org/apache/samza/config/ElasticsearchConfig.java <https://reviews.apache.org/r/33297/#comment133087> those are general methods. I think there are two ways for this: 1. reuse existing method, in samza-core/src/main/scala/org/apache/samza/util/Util.scala - getObj 2. put it to samza-core/src/main/java/org/apache/samza/util/Util.java (it does not exist now) samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java <https://reviews.apache.org/r/33297/#comment133090> change -> changed samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/ClientFactory.java <https://reviews.apache.org/r/33297/#comment133089> give it an "elastic" prefix? samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/TransportClientFactory.java <https://reviews.apache.org/r/33297/#comment133088> can those be more elastic-specific? "host" and "port" are too general. samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/BulkProcessorFactory.java <https://reviews.apache.org/r/33297/#comment133084> where is this entrySet() from? samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/NodeClientFactoryTest.java <https://reviews.apache.org/r/33297/#comment133091> this only test the factory gets the config, does not test the GetClient method works correctly, right? samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/TransportClientFactoryTest.java <https://reviews.apache.org/r/33297/#comment133092> same as previous comment Overall, looks good for me. thought: Can we put all the config in the ElasticConfig class, not spread all to the Factory classes? Then it will be eaiser to manager -- we know how many configs needed by elastic, what is set, what is read, etc. Also The ElasticConfig should only be responsible for reading the config and we create factories in other classes, such as in the Producer class, if needed. What do you think? - Yan Fang On May 1, 2015, 6:23 p.m., Dan Harvey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33297/ > ----------------------------------------------------------- > > (Updated May 1, 2015, 6:23 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > [SAMZA-654] Added ElasticsearchSystemProducer and Factory to output messages > into Elasticseach indexes. > > > Diffs > ----- > > build.gradle 97de3a28f6379e3862eec845da87587b1d4f742e > docs/learn/documentation/versioned/jobs/configuration-table.html > 5ebe8a77bc3284de43268655897df33c003d56d6 > gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e > > samza-elasticsearch/src/main/java/org/apache/samza/config/ElasticsearchConfig.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/BulkProcessorFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemAdmin.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/ClientFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/NodeClientFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/TransportClientFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java > PRE-CREATION > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/IndexRequestFactory.java > PRE-CREATION > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java > PRE-CREATION > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/NodeClientFactoryTest.java > PRE-CREATION > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/TransportClientFactoryTest.java > PRE-CREATION > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactoryTest.java > PRE-CREATION > settings.gradle bb07a3b84b14dcef94da1bb166eab6aa3d0026bb > > Diff: https://reviews.apache.org/r/33297/diff/ > > > Testing > ------- > > > Thanks, > > Dan Harvey > >
