You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sschaef <gi...@git.apache.org> on 2017/05/17 21:38:15 UTC
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
GitHub user sschaef opened a pull request:
https://github.com/apache/flink/pull/3934
[FLINK-6065] Add initClient method to ElasticsearchApiCallBridge
This adds the method `initClient` to `ElasticsearchApiCallBridge` in
order to resolve FLINK-6065. This new method takes as argument a
function that can create a `TransportClient`. This is required in order
to not hardcode the `TransportClient` in the implementation of
`createClient`. `createClient` continues to exist in order to allow
backwards compatibility.
No tests provided because I couldn't find any existing test class that
tests the implementation of `ElasticsearchApiCallBridge`.
------------------------------------------------------------------------
This is my first contribution, I haven't signed the ICLA yet (which I will do next). I didn't provide any tests yet because I had no idea how the functionality should be tested or where a test class should be added. If you would like to see tests for the changes, please tell me.
The change fixes the ticket but maybe you would like to see a different way on how to fix the issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sschaef/flink flink-6065
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3934.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3934
----
commit 2815b0c9ca407838611ba60b9c35a0ac550005e3
Author: Simon Schäfer <ma...@antoras.de>
Date: 2017-05-17T20:24:14Z
[FLINK-6065] Add initClient method to ElasticsearchApiCallBridge
This adds the method `initClient` to `ElasticsearchApiCallBridge` in
order to resolve FLINK-6065. This new method takes as argument a
function that can create a `TransportClient`. This is required in order
to not hardcode the `TransportClient` in the implementation of
`createClient`. `createClient` continues to exist in order to allow
backwards compatibility.
No tests provided because I couldn't find any existing test class that
tests the implementation of `ElasticsearchApiCallBridge`.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3934: [FLINK-6065] Add initClient method to ElasticsearchApiCal...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3934
Hi @sschaef! I was wondering if you would like to finish this work? Either way, please let me know :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/3934#discussion_r117451847
--- Diff: flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/ElasticsearchApiCallBridge.java ---
@@ -40,12 +43,27 @@
/**
* Creates an Elasticsearch {@link Client}.
*
+ * In comparison to {@link initClient}, this method creates a default {@link Client}.
+ *
* @param clientConfig The configuration to use when constructing the client.
* @return The created client.
*/
Client createClient(Map<String, String> clientConfig);
/**
+ * Initializes an Elasticsearch {@link Client}.
+ *
+ * A {@link Settings} object is created, which is passed to {@link mapper} in order to allow the creation of a
+ * {@link TransportClient}. {@link createClient} creates and initializes a default client for cases where the
+ * implementation doesn't matter.
+ *
+ * @param clientConfig The configuration to use when constructing the client.
+ * @param mapper Receives a {@link Settings} object that can be used to create a {@link TransportClient}.
+ * @return The initialized client that has been created by {@link mapper}.
+ */
+ Client initClient(Map<String, String> clientConfig, Function<Settings, TransportClient> mapper);
--- End diff --
I wonder if a `TransportClientFactory` would be more intuitive here? That would also solve the Java 8 problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3934: [FLINK-6065] Add initClient method to ElasticsearchApiCal...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3934
Answer to some of your other questions:
1. I don't think the Apache ICLA is strictly required, unless you're invited as a Committer of the project.
2. The tests should be added under `src/test/...` directory of the module. I think the already-existing IT tests for the ES sinks already cover the changed functionality here. Since this isn't a very complicated feature, for new tests, at the very most I would suggest perhaps to add unit tests that test the factory is correctly respected during the init flow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3934: [FLINK-6065] Add initClient method to ElasticsearchApiCal...
Posted by sschaef <gi...@git.apache.org>.
Github user sschaef commented on the issue:
https://github.com/apache/flink/pull/3934
The Java7 run failed because my solution uses Java8 features. I guess I have to find another way to fix this issue then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/3934#discussion_r117452728
--- Diff: flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/ElasticsearchApiCallBridge.java ---
@@ -40,12 +43,27 @@
/**
* Creates an Elasticsearch {@link Client}.
*
+ * In comparison to {@link initClient}, this method creates a default {@link Client}.
+ *
* @param clientConfig The configuration to use when constructing the client.
* @return The created client.
*/
Client createClient(Map<String, String> clientConfig);
--- End diff --
The `ElasticsearchApiCallBridge` actually isn't a user-facing class, only an interface for our version-specific implementations. So I think it is ok to alter the interface freely.
In that case, it shouldn't be necessary to have a new method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/3934#discussion_r117455309
--- Diff: flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/ElasticsearchApiCallBridge.java ---
@@ -40,12 +43,27 @@
/**
* Creates an Elasticsearch {@link Client}.
*
+ * In comparison to {@link initClient}, this method creates a default {@link Client}.
+ *
* @param clientConfig The configuration to use when constructing the client.
* @return The created client.
*/
Client createClient(Map<String, String> clientConfig);
/**
+ * Initializes an Elasticsearch {@link Client}.
+ *
+ * A {@link Settings} object is created, which is passed to {@link mapper} in order to allow the creation of a
+ * {@link TransportClient}. {@link createClient} creates and initializes a default client for cases where the
+ * implementation doesn't matter.
+ *
+ * @param clientConfig The configuration to use when constructing the client.
+ * @param mapper Receives a {@link Settings} object that can be used to create a {@link TransportClient}.
+ * @return The initialized client that has been created by {@link mapper}.
+ */
+ Client initClient(Map<String, String> clientConfig, Function<Settings, TransportClient> mapper);
--- End diff --
As for the defaults, a default `DefaultTransportClientFactory` that replaces the previous hardcodes can be provided to this method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
Posted by sschaef <gi...@git.apache.org>.
Github user sschaef closed the pull request at:
https://github.com/apache/flink/pull/3934
---
[GitHub] flink pull request #3934: [FLINK-6065] Add initClient method to Elasticsearc...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/3934#discussion_r117452130
--- Diff: flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/ElasticsearchApiCallBridge.java ---
@@ -40,12 +43,27 @@
/**
* Creates an Elasticsearch {@link Client}.
*
+ * In comparison to {@link initClient}, this method creates a default {@link Client}.
--- End diff --
I don't think the `{@link initClient}` Javadoc link have the correct signature, does it?
For example, `{@link initClient}` should be `{@link #initClient(Map<String, String>, Function<Settings, TransportClient>)}` to actually link to that method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---