You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tweise <gi...@git.apache.org> on 2018/04/22 06:43:59 UTC
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
GitHub user tweise opened a pull request:
https://github.com/apache/flink/pull/5889
[FLINK-9188] [kinesis] Generic mechanism to set ClientConfiguration properties.
## What is the purpose of the change
This pull request enables setting properties on the AWS ClientConfiguration from user supplied properties with a specific prefix.
## Brief change log
- use Jackson to set properties in a generic way so that we don't need to assume knowledge of the AWS properties in the Flink connector code.
## Verifying this change
This change added tests and can be verified as follows:
Added test to verify the configuration mechanism.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tweise/flink FLINK-9188.ConfigureKinesisClient
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5889.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 #5889
----
commit b65ffd5a3fd72478c7cfdaff17b4199febaa7650
Author: Thomas Weise <th...@...>
Date: 2018-04-17T05:01:52Z
[FLINK-9188] [kinesis] Generic mechanism to set ClientConfiguration properties.
----
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/5889#discussion_r183966110
--- Diff: flink-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/proxy/KinesisProxy.java ---
@@ -186,7 +186,10 @@ protected KinesisProxy(Properties configProps) {
* @return
*/
protected AmazonKinesis createKinesisClient(Properties configProps) {
- return AWSUtil.createKinesisClient(configProps, new ClientConfigurationFactory().getConfig());
+
+ ClientConfiguration awsClientConfig = new ClientConfigurationFactory().getConfig();
+ AWSUtil.setAwsClientConfigProperties(awsClientConfig, configProps);
+ return AWSUtil.createKinesisClient(configProps, awsClientConfig);
--- End diff --
Currently, the explicit Flink-defined keys in `AWSConfigConstants` will override whatever is set via the generic mechanism, correct?
---
[GitHub] flink issue #5889: [FLINK-9188] [kinesis] Generic mechanism to set ClientCon...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the issue:
https://github.com/apache/flink/pull/5889
R: @tzulitai
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/flink/pull/5889#discussion_r184220854
--- Diff: flink-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/proxy/KinesisProxy.java ---
@@ -186,7 +186,10 @@ protected KinesisProxy(Properties configProps) {
* @return
*/
protected AmazonKinesis createKinesisClient(Properties configProps) {
- return AWSUtil.createKinesisClient(configProps, new ClientConfigurationFactory().getConfig());
+
+ ClientConfiguration awsClientConfig = new ClientConfigurationFactory().getConfig();
+ AWSUtil.setAwsClientConfigProperties(awsClientConfig, configProps);
+ return AWSUtil.createKinesisClient(configProps, awsClientConfig);
--- End diff --
Actually there does not seem to be any overlap. `AWSConfigConstants.AWS_REGION` and `AWSConfigConstants.AWS_ENDPOINT` are used to construct the client, not the client config.
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/flink/pull/5889#discussion_r184218974
--- Diff: flink-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/proxy/KinesisProxy.java ---
@@ -186,7 +186,10 @@ protected KinesisProxy(Properties configProps) {
* @return
*/
protected AmazonKinesis createKinesisClient(Properties configProps) {
- return AWSUtil.createKinesisClient(configProps, new ClientConfigurationFactory().getConfig());
+
+ ClientConfiguration awsClientConfig = new ClientConfigurationFactory().getConfig();
+ AWSUtil.setAwsClientConfigProperties(awsClientConfig, configProps);
+ return AWSUtil.createKinesisClient(configProps, awsClientConfig);
--- End diff --
That's correct, the generic property setting takes place before those keys are processed.
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/5889#discussion_r183965718
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/proxy/KinesisProxyTest.java ---
@@ -86,4 +86,19 @@ protected AmazonKinesis createKinesisClient(Properties configProps) {
assertEquals(10000, clientConfiguration.getSocketTimeout());
}
+ @Test
+ public void testClientConfigOverride() {
+
+ Properties configProps = new Properties();
+ configProps.setProperty(AWSConfigConstants.AWS_REGION, "us-east-1");
+ configProps.setProperty(AWSUtil.AWS_CLIENT_CONFIG_PREFIX + "socketTimeout", "9999");
--- End diff --
Would this usage also subsume other commonly seen settings, such as setting the AWS crredentials?
Currently we have these explicitly defined keys in `AWSConfigConstants`, for which we might be able to get rid of (because we have to maintain the set of config keys ourselves, which is not nice and hard to meet with all the functionalities that KCL provides).
If so, we might also want tests + update our documents for those.
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/flink/pull/5889#discussion_r184224413
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/proxy/KinesisProxyTest.java ---
@@ -86,4 +86,19 @@ protected AmazonKinesis createKinesisClient(Properties configProps) {
assertEquals(10000, clientConfiguration.getSocketTimeout());
}
+ @Test
+ public void testClientConfigOverride() {
+
+ Properties configProps = new Properties();
+ configProps.setProperty(AWSConfigConstants.AWS_REGION, "us-east-1");
+ configProps.setProperty(AWSUtil.AWS_CLIENT_CONFIG_PREFIX + "socketTimeout", "9999");
--- End diff --
The credentials construction code may be hard to replace declaratively. Also, the credentials are not a client config property, but a property of the Kinesis client.
---
[GitHub] flink pull request #5889: [FLINK-9188] [kinesis] Generic mechanism to set Cl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5889
---
[GitHub] flink issue #5889: [FLINK-9188] [kinesis] Generic mechanism to set ClientCon...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/5889
Thanks for the comments @tweise!
Looks good to me, will proceed to merge this ..
---