You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Bastian Germann <ba...@fishpost.de> on 2014/08/03 14:22:27 UTC
Review Request 24213: FLUME-2434 elasticsearchsink: class name as clientType
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/
-----------------------------------------------------------
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
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
> 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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
> On Aug. 3, 2014, 4:42 p.m., Pawe? 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? ?
Right, that is the new feature provided by this changeset. It is less readable, but it is required for that feature.
- 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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Pawel <pa...@gmail.com>.
> On Aug. 3, 2014, 2: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.
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
}
- Pawel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review49429
-----------------------------------------------------------
On Aug. 3, 2014, 12: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, 12: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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Pawe? <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review49429
-----------------------------------------------------------
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClientFactory.java
<https://reviews.apache.org/r/24213/#comment86497>
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? ?
- Pawe?
On Aug. 3, 2014, 12: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, 12: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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review52719
-----------------------------------------------------------
Ship it!
Ship It!
- Bastian Germann
On Aug. 12, 2014, 12:44 nachm., Bastian Germann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24213/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 12:44 nachm.)
>
>
> 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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Edward Sargisson <es...@pobox.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review50888
-----------------------------------------------------------
Ship it!
I'm happy with this now.
- Edward Sargisson
On Aug. 12, 2014, 3:44 a.m., Bastian Germann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24213/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 3:44 a.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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/
-----------------------------------------------------------
(Updated Aug. 12, 2014, 12:44 p.m.)
Review request for Flume.
Changes
-------
Added statements about the interfaces in the user guide.
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 (updated)
-----
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
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
> On Aug. 11, 2014, 8:41 p.m., Edward Sargisson wrote:
> > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java, line 326
> > <https://reviews.apache.org/r/24213/diff/2/?file=651830#file651830line326>
> >
> > What is the purpose of the factory if we then take its output and do further configuration on it?
> >
> > Can we not have the factory do the complete initialisation or is there a reason not to?
For testing purposes you can set a local transport client, which would override the factory. To have the further configuration passed to it, I placed it outside the factory.
- Bastian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review50203
-----------------------------------------------------------
On Aug. 12, 2014, 12:44 p.m., Bastian Germann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24213/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 12:44 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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Edward Sargisson <es...@pobox.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/#review50203
-----------------------------------------------------------
I have a small question about the division of responsibilities with the factory and some minor doc updates.
Otherwise this is a useful and clean change.
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/24213/#comment87813>
I think we should state what interface the indexNameBuilder has to implement.
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/24213/#comment87814>
I think we should state what interface the ElasticSearchClient has to implement.
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java
<https://reviews.apache.org/r/24213/#comment87818>
What is the purpose of the factory if we then take its output and do further configuration on it?
Can we not have the factory do the complete initialisation or is there a reason not to?
- Edward Sargisson
On Aug. 5, 2014, 3:13 a.m., Bastian Germann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24213/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 3:13 a.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
>
>
Re: Review Request 24213: FLUME-2434 elasticsearchsink: class name as
clientType
Posted by Bastian Germann <ba...@fishpost.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24213/
-----------------------------------------------------------
(Updated Aug. 5, 2014, 12:13 p.m.)
Review request for Flume.
Changes
-------
Using the constructors of the provided client implementations directly.
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 (updated)
-----
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