> On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/config/ElasticsearchConfig.java, > > lines 93-110 > > <https://reviews.apache.org/r/33297/diff/4-5/?file=940522#file940522line93> > > > > 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)
I tried to add the Java util, but that collides with the Scala class name, so I just added another method to the Scala util instead. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 54 > > <https://reviews.apache.org/r/33297/diff/4-5/?file=940525#file940525line54> > > > > change -> changed Fixed. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/ClientFactory.java, > > line 43 > > <https://reviews.apache.org/r/33297/diff/4-5/?file=940526#file940526line43> > > > > give it an "elastic" prefix? Added. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/client/TransportClientFactory.java, > > lines 47-53 > > <https://reviews.apache.org/r/33297/diff/4-5/?file=940528#file940528line47> > > > > can those be more elastic-specific? "host" and "port" are too general. Updated. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/BulkProcessorFactory.java, > > line 74 > > <https://reviews.apache.org/r/33297/diff/5/?file=947308#file947308line74> > > > > where is this entrySet() from? The Factory is an instance of Config, but that's all changing now. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/NodeClientFactoryTest.java, > > line 33 > > <https://reviews.apache.org/r/33297/diff/5/?file=947318#file947318line33> > > > > this only test the factory gets the config, does not test the GetClient > > method works correctly, right? Yes, with the config update I've fixed this. > On May 4, 2015, 6:53 a.m., Yan Fang wrote: > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/client/TransportClientFactoryTest.java, > > line 33 > > <https://reviews.apache.org/r/33297/diff/5/?file=947319#file947319line33> > > > > same as previous comment Fixed. On May 4, 2015, 6:53 a.m., Dan Harvey wrote: > > 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? Yes that makes sense. I've moved all config into ElasticsearchConfig and put the factory code where it is used. It does seem cleaner now and easier to check all the config. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33297/#review82358 ----------------------------------------------------------- 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 > >
