Feature: Add KPL/KCL support #75

r1m · 2019-01-03T16:54:23Z

This depends on spring-projects/spring-integration-aws#103.

Add support for KCL and KPL kinesis library fixes #65.

artembilan

That's it for the current round.

Thanks

...ngframework/cloud/stream/binder/kinesis/properties/KinesisBinderConfigurationProperties.java

...c/main/java/org/springframework/cloud/stream/binder/kinesis/KinesisMessageChannelBinder.java

artembilan · 2019-03-14T21:42:43Z

@r1m ,

May we come back to this feature since Spring Integration AWS is now based on KCL v1: spring-projects/spring-integration-aws@dd9f3a7 ?

Thanks

javarno · 2019-03-15T08:22:18Z

I had already done part of the changes on my local git repo, I'll rebase to get the latest modifications and try to finish the requested changes.

codecov · 2019-03-15T13:42:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@194297e). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master      #75   +/-   ##
=========================================
  Coverage          ?   24.73%           
  Complexity        ?       22           
=========================================
  Files             ?       10           
  Lines             ?      473           
  Branches          ?       36           
=========================================
  Hits              ?      117           
  Misses            ?      345           
  Partials          ?       11

Impacted Files	Coverage Δ	Complexity Δ
...der/kinesis/config/KinesisBinderConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...am/binder/kinesis/KinesisMessageChannelBinder.java	`0% <0%> (ø)`	`0 <0> (?)`
...operties/KinesisBinderConfigurationProperties.java	`35.41% <0%> (ø)`	`7 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 194297e...5f06ddf. Read the comment docs.

javarno · 2019-03-15T13:46:39Z

I think I covered all the requested changes, it would be nice to have your feedback on this.

artembilan

Would you mind to rebase your branch to the latest master?

It is hard to review those files which are already in the master and are not a part of your change

Thanks

artembilan · 2019-03-15T17:04:03Z

pom.xml

 		<relativePath/>
 	</parent>

 	<artifactId>spring-cloud-stream-binder-kinesis-parent</artifactId>
-	<version>1.1.0.BUILD-SNAPSHOT</version>
+	<version>1.1.1.BUILD-SNAPSHOT</version>


Well, that's not. Since it is about a new feature in the project, so we will go a new minor release. Therefore: 1.2.0.BUILD-SNAPSHOT

relates to spring-cloud#65, spring-cloud#53

javarno · 2019-03-15T18:14:20Z

Sorry about that, I'm still quite new to git and not very familiar with the rebase (especially on an upstream repo), I hope this push fixes it.

artembilan

That's all. nothing critical.

Very close to the merge.

Please, consider to update Copyrights for all the affected classes to the current 2019 year.

artembilan · 2019-03-15T18:27:54Z

spring-cloud-stream-binder-kinesis/pom.xml

-
+	<properties>
+		<amazon-kinesis-client.version>1.9.3</amazon-kinesis-client.version>
+		<amazon-kinesis-producer.version>0.12.11</amazon-kinesis-producer.version>


These must go to the root pom as a dependencyManagement, alongside with other managed in this project dependencies. So, this pom won't suffer from missed properties.

artembilan · 2019-03-15T18:48:41Z

.../java/org/springframework/cloud/stream/binder/kinesis/config/KinesisBinderConfiguration.java

+	@Bean(name = "kclTaskExecutor")
+	@ConditionalOnMissingBean(name = "kclTaskExecutor")
+	@ConditionalOnProperty(name = "spring.cloud.stream.kinesis.binder.kpl-kcl-enabled", havingValue = "true")
+	public TaskExecutor kclTaskExecutor() {


I don't think we need this bean.

For this kind of Microservices, based on Spring Cloud Stream, it is fully enough to rely on the internal TaskExecutor.

So, please, remove.

artembilan · 2019-03-15T19:01:19Z

...ngframework/cloud/stream/binder/kinesis/properties/KinesisBinderConfigurationProperties.java

@@ -44,6 +44,8 @@

 	private int minShardCount = 1;

+	private boolean kplKclEnabled;


Please, add short descriptive JavaDoc for this field. With period in the end.
This is going to help tooling and docs to generate helpful metadata.

I understand that all other properties are without that, but this is slightly different story which we definitely should fix too. Eventually...

artembilan · 2019-03-15T19:02:08Z

...ngframework/cloud/stream/binder/kinesis/properties/KinesisBinderConfigurationProperties.java

@@ -121,6 +131,8 @@ public void setAutoCreateStream(boolean autoCreateStream) {

 		private Integer timeToLive;

+		private Long interval = null;


The same JavaDocs description, please, here.
I don't think we need this = null

...c/main/java/org/springframework/cloud/stream/binder/kinesis/KinesisMessageChannelBinder.java

javarno · 2019-03-18T14:38:38Z

Changes done. For the task executor, I had to use a qualifier since there are two TaskExecutor beans available, I hope I chose the right one.

artembilan

Review for the next round.

artembilan · 2019-03-18T18:12:38Z

.../java/org/springframework/cloud/stream/binder/kinesis/config/KinesisBinderConfiguration.java

@@ -93,7 +92,7 @@ public KinesisMessageChannelBinder kinesisMessageChannelBinder(
 			KinesisExtendedBindingProperties kinesisExtendedBindingProperties,
 			@Autowired(required = false) KinesisProducerConfiguration kinesisProducerConfiguration,
 			AWSCredentialsProvider awsCredentialsProvider,
-			@Autowired(required = false) @Qualifier("kclTaskExecutor") TaskExecutor kclTaskExecutor) {
+			@Autowired(required = false) @Qualifier("taskScheduler") TaskExecutor kclTaskExecutor) {


No, my point is do not inject any TaskExecutor at all and just rely on the default one created in the channel adapter.

artembilan · 2019-03-18T18:12:48Z

.../java/org/springframework/cloud/stream/binder/kinesis/config/KinesisBinderConfiguration.java

+//	@ConditionalOnProperty(name = "spring.cloud.stream.kinesis.binder.kpl-kcl-enabled", havingValue = "true")
+//	public TaskExecutor kclTaskExecutor() {
+//		return new SimpleAsyncTaskExecutor();
+//	}


Please, remove this code altogether.

artembilan · 2019-03-18T18:14:07Z

...ngframework/cloud/stream/binder/kinesis/properties/KinesisBinderConfigurationProperties.java

@@ -44,6 +44,9 @@

 	private int minShardCount = 1;

+	/** Enables the usage of Amazon KCL/KPL libraries for all message consumption and production. */


This is not a JavaDoc.
Please, consider to have one more line in this description:

Suggested change

/** Enables the usage of Amazon KCL/KPL libraries for all message consumption and production. */

/**

* Enables the usage of Amazon KCL/KPL libraries for all message consumption and production.

*/

artembilan · 2019-03-18T18:14:30Z

...ngframework/cloud/stream/binder/kinesis/properties/KinesisBinderConfigurationProperties.java

@@ -121,6 +132,9 @@ public void setAutoCreateStream(boolean autoCreateStream) {

 		private Integer timeToLive;

+		/** Interval between two checkpoints when checkpoint mode is periodic. */


DITTO.
Please, see other similar JavaDocs in this class.

javarno · 2019-03-19T09:06:49Z

Sorry, I misunderstood about the task executor, it should be all fixed now.

artembilan

LGTM!

Pulling locally for final review...

artembilan · 2019-03-19T18:52:11Z

Merged as 2843ded

@javarno ,

Thank you very much for the contribution; looking forward for more!

javarno · 2019-03-19T20:36:32Z

Great, thanks for all the help and code reviews.

sabbyanandan added the in pr label Jan 3, 2019

sabbyanandan assigned artembilan Jan 9, 2019

artembilan requested changes Feb 11, 2019

View reviewed changes

artembilan requested changes Mar 15, 2019

View reviewed changes

Feature: use KCL/KPL message handler and channel adapter

45f3494

relates to spring-cloud#65, spring-cloud#53

javarno force-pushed the feature/kpl-0.12.11-kcl-2.0.5 branch from 2e52077 to 45f3494 Compare March 15, 2019 18:12

artembilan requested changes Mar 15, 2019

View reviewed changes

requested changes for KPL/KCL support merge request

d51fe1a

artembilan requested changes Mar 18, 2019

View reviewed changes

requested changes for KPL/KCL support merge request

5f06ddf

artembilan approved these changes Mar 19, 2019

View reviewed changes

artembilan closed this Mar 19, 2019

sabbyanandan removed the in pr label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add KPL/KCL support #75

Feature: Add KPL/KCL support #75

r1m commented Jan 3, 2019

artembilan left a comment

artembilan commented Mar 14, 2019

javarno commented Mar 15, 2019

codecov bot commented Mar 15, 2019 •

edited

javarno commented Mar 15, 2019

artembilan left a comment

artembilan Mar 15, 2019

javarno commented Mar 15, 2019

artembilan left a comment

artembilan Mar 15, 2019

artembilan Mar 15, 2019

artembilan Mar 15, 2019

artembilan Mar 15, 2019

javarno commented Mar 18, 2019

artembilan left a comment

artembilan Mar 18, 2019

artembilan Mar 18, 2019

artembilan Mar 18, 2019

artembilan Mar 18, 2019

javarno commented Mar 19, 2019

artembilan left a comment

artembilan commented Mar 19, 2019

javarno commented Mar 19, 2019

		@@ -44,6 +44,8 @@

		private int minShardCount = 1;

		private boolean kplKclEnabled;

		@@ -121,6 +131,8 @@ public void setAutoCreateStream(boolean autoCreateStream) {

		private Integer timeToLive;

		private Long interval = null;

		@@ -44,6 +44,9 @@

		private int minShardCount = 1;

		/** Enables the usage of Amazon KCL/KPL libraries for all message consumption and production. */

		@@ -121,6 +132,9 @@ public void setAutoCreateStream(boolean autoCreateStream) {

		private Integer timeToLive;

		/** Interval between two checkpoints when checkpoint mode is periodic. */

Feature: Add KPL/KCL support #75

Feature: Add KPL/KCL support #75

Conversation

r1m commented Jan 3, 2019

artembilan left a comment

Choose a reason for hiding this comment

artembilan commented Mar 14, 2019

javarno commented Mar 15, 2019

codecov bot commented Mar 15, 2019 • edited

Codecov Report

javarno commented Mar 15, 2019

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javarno commented Mar 15, 2019

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javarno commented Mar 18, 2019

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javarno commented Mar 19, 2019

artembilan left a comment

Choose a reason for hiding this comment

artembilan commented Mar 19, 2019

javarno commented Mar 19, 2019

codecov bot commented Mar 15, 2019 •

edited