ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs #619

a1shadows · 2024-01-28T14:14:46Z

New Feature: Support to consume on all partitions from specified timestamp/epoch

PR Branch
https://github.com/a1shadows/zerocode/tree/issue-615/seekToTimestamp

fixes #615
fixes #621

Motivation and Context

Right now, the seek method available can only seek from one partition and has no semantics to consume from a specific timestamp. It only consumes the last 'n' messages on the specified partition.

With this new feature, we will be able to consume on all partitions from specified timestamp/epoch in the following fashion:

"consumerLocalConfigs" : {
    "seekTimestamp" : {
      "timestamp" : "2024-02-04T14:53:20.071",
      "format" : "yyyy-MM-dd'T'HH:mm:ss.SSS"
    }

or like this:

"consumerLocalConfigs" : {
    "seekEpoch" : "1707038600071"
  }

in the kafka consume step.

In addition, we have also removed "seek" from ConsumerCommonConfigs as it made no sense in a common context and can now only be specified as a consumerLocalConfig.

Checklist:

core/src/main/java/org/jsmart/zerocode/core/kafka/consume/ConsumerLocalConfigs.java

1. Changed epoch seekTimestamp to seekEpoch and added seekTimestamp with format and timestamp 2. Added validations for seekEpoch and seekTimestamp 3. Implemented seekTimestamp feature

…issue-615/seekToTimestamp

a1shadows · 2024-02-04T09:30:57Z

@nirmalchandra Which Wiki page should I update with details of use of this feature? Have added a ser/des unit test and an integration test for the feature which is working as expected

a1shadows · 2024-02-07T08:13:57Z

@nirmalchandra @authorjapps @M3lkior hey, let me know if there is any action i need to take on this

authorjapps · 2024-02-07T08:51:17Z

pom.xml

@@ -138,6 +139,13 @@
                <artifactId>json</artifactId>
                <version>20160810</version>
            </dependency>
+            <dependency>
+                <groupId>org.projectlombok</groupId>


Hi @a1shadows , Great work!

It's preferred to go with simple getter/setter methods, without using lombok.
This has caused IDE dependency, readability and other issues in the past.
Can you please remove this ?

Rest everything looks amazing. 🚀
I might add couple of more scenarios to the test pack before merging.

@authorjapps

I added Lombok to reduce the boilerplate code. Are you sure we want to write explicit getters, setters and constructors for everything?

At my workplace, we have been using Lombok extensively and at great scale with no issues faced so far, and the code stays compact.

Let me know either way and I'll make the changes

@authorjapps

I added Lombok to reduce the boilerplate code. Are you sure we want to write explicit getters, setters and constructors for everything?

At my workplace, we have been using Lombok extensively and at great scale with no issues faced so far, and the code stays compact.

Let me know either way and I'll make the changes

Thanks for understanding @a1shadows . Kindly make the changes to remove lombok.

Reason is simple, it has happened in the past, worth thinking about the practicality of the issue in terms of benefits.
Thousands of projects are using this library and we wouldn't want to have additional maintenance overhead due to some projects at some corner of the world may comeback with issues, wanting us to fix it.
That will need additional work to reproduce the issue, fix, PR and merge, then release cycle.

Also, can you take a look and fix this CI failure? 🔴
https://github.com/authorjapps/zerocode/pull/619/checks#step:6:12150 (click)

testSerDeser_seekEpoch(org.jsmart.zerocode.core.kafka.consume.ConsumerLocalConfigsWrapTest) Time elapsed: 0.004 sec <<< FAILURE! java.lang.AssertionError: consumerLocalConfigs.seekEpoch Expected: 1706940293669 got: 1707299617296 at org.skyscreamer.jsonassert.JSONAssert.assertEquals(JSONAssert.java:417) at org.skyscreamer.jsonassert.JSONAssert.assertEquals(JSONAssert.java:394) at org.jsmart.zerocode.core.kafka.consume.ConsumerLocalConfigsWrapTest.testSerDeser_seekEpoch(ConsumerLocalConfigsWrapTest.java:36)

@authorjapps could you let me know on the above point? Also, can you let me know when the PR looks ready to merge? Would love to see a merge commit on my name ? 😁

@authorjapps could you let me know on the above point? Also, can you let me know when the PR looks ready to merge? Would love to see a merge commit on my name ? 😁

Thanks for understanding @a1shadows .
Can you please make the changes to remove lombok dependency soon you get chance?
Once done, please confirm.

@authorjapps Did you have time to consider Lombok use in the project? We can whittle down the volume of code significantly in the long run if we adopt it. Let me know.

@authorjapps Did you have time to consider Lombok use in the project? We can whittle down the volume of code significantly in the long run if we adopt it. Let me know.

Already provided my view in this thread.

To add more clarity:
@a1shadows , Lets do it this way to derisk this release if you are keen in using lombok.

For this PR, please remove it and lets release it (version = 1.0.36 )

Raise a new PR with it and we can make a new release with lombok (e.g. version = 1.0.37-RC-lombok )
(Use this new Issue No. 624 for your commits. ) Release with Lombok dependency (with a RC version) #624

This way we don't introduce overhead with our regular releases, then once the version 1.0.37-RC-lombok becomes stable, we can merge the code to main release.

Can you amend this PR for Point No.1 please?
and
a new PR for Point No.2 please? <--- This is optional as it it is only Dev stuff, really doesn't affect any users

@authorjapps Hey, I have removed the Lombok dependency and fixed the test. Can you take another look at this?

@authorjapps Hey, I have removed the Lombok dependency and fixed the test. Can you take another look at this?

Thanks, just given a rerun now on the CI job. Once Green, we should be able to merge.

@authorjapps Looks like CI is a success. Can we merge now?

core/src/main/java/org/jsmart/zerocode/core/kafka/helper/KafkaConsumerHelper.java

authorjapps · 2024-02-07T20:21:26Z

pom.xml

@@ -138,6 +139,13 @@
                <artifactId>json</artifactId>
                <version>20160810</version>
            </dependency>
+            <dependency>
+                <groupId>org.projectlombok</groupId>


@authorjapps

I added Lombok to reduce the boilerplate code. Are you sure we want to write explicit getters, setters and constructors for everything?

At my workplace, we have been using Lombok extensively and at great scale with no issues faced so far, and the code stays compact.

Let me know either way and I'll make the changes

Thanks for understanding @a1shadows . Kindly make the changes to remove lombok.

Reason is simple, it has happened in the past, worth thinking about the practicality of the issue in terms of benefits.
Thousands of projects are using this library and we wouldn't want to have additional maintenance overhead due to some projects at some corner of the world may comeback with issues, wanting us to fix it.
That will need additional work to reproduce the issue, fix, PR and merge, then release cycle.

Also, can you take a look and fix this CI failure? 🔴
https://github.com/authorjapps/zerocode/pull/619/checks#step:6:12150 (click)

testSerDeser_seekEpoch(org.jsmart.zerocode.core.kafka.consume.ConsumerLocalConfigsWrapTest) Time elapsed: 0.004 sec <<< FAILURE! java.lang.AssertionError: consumerLocalConfigs.seekEpoch Expected: 1706940293669 got: 1707299617296 at org.skyscreamer.jsonassert.JSONAssert.assertEquals(JSONAssert.java:417) at org.skyscreamer.jsonassert.JSONAssert.assertEquals(JSONAssert.java:394) at org.jsmart.zerocode.core.kafka.consume.ConsumerLocalConfigsWrapTest.testSerDeser_seekEpoch(ConsumerLocalConfigsWrapTest.java:36)

authorjapps · 2024-02-09T14:54:55Z

pom.xml

@@ -138,6 +139,13 @@
                <artifactId>json</artifactId>
                <version>20160810</version>
            </dependency>
+            <dependency>
+                <groupId>org.projectlombok</groupId>


@authorjapps could you let me know on the above point? Also, can you let me know when the PR looks ready to merge? Would love to see a merge commit on my name ? 😁

Thanks for understanding @a1shadows .
Can you please make the changes to remove lombok dependency soon you get chance?
Once done, please confirm.

authorjapps · 2024-02-11T18:24:42Z

pom.xml

@@ -138,6 +139,13 @@
                <artifactId>json</artifactId>
                <version>20160810</version>
            </dependency>
+            <dependency>
+                <groupId>org.projectlombok</groupId>


@authorjapps Did you have time to consider Lombok use in the project? We can whittle down the volume of code significantly in the long run if we adopt it. Let me know.

Already provided my view in this thread.

To add more clarity:
@a1shadows , Lets do it this way to derisk this release if you are keen in using lombok.

For this PR, please remove it and lets release it (version = 1.0.36 )

Raise a new PR with it and we can make a new release with lombok (e.g. version = 1.0.37-RC-lombok )
(Use this new Issue No. 624 for your commits. ) Release with Lombok dependency (with a RC version) #624

This way we don't introduce overhead with our regular releases, then once the version 1.0.37-RC-lombok becomes stable, we can merge the code to main release.

Can you amend this PR for Point No.1 please?
and
a new PR for Point No.2 please? <--- This is optional as it it is only Dev stuff, really doesn't affect any users

nirmalchandra

Good work. Approved

Changes to support seekFromTimestamp

b414c65

a1shadows mentioned this pull request Jan 28, 2024

Feature Suggestion: Kafka Consumer consume from timestamp #615

Closed

Merge branch 'master' into issue-615/seekToTimestamp

b6df9d7

nirmalchandra reviewed Jan 31, 2024

View reviewed changes

core/src/main/java/org/jsmart/zerocode/core/kafka/consume/ConsumerLocalConfigs.java Outdated Show resolved Hide resolved

a1shadows added 3 commits February 3, 2024 09:07

seekTimestamp added with format

0700b3f

1. Changed epoch seekTimestamp to seekEpoch and added seekTimestamp with format and timestamp 2. Added validations for seekEpoch and seekTimestamp 3. Implemented seekTimestamp feature

Merge remote-tracking branch 'origin/issue-615/seekToTimestamp' into …

101a270

…issue-615/seekToTimestamp

added kafka-testing cases for seekEpoch and seekTimestamp

f01e108

a1shadows marked this pull request as ready for review February 4, 2024 09:30

a1shadows changed the title ~~ISSUE 615: Changes to support seekFromTimestamp in KafkaReceiver~~ ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs Feb 4, 2024

Merge branch 'master' into issue-615/seekToTimestamp

fec6c05

a1shadows requested a review from nirmalchandra February 4, 2024 09:37

authorjapps reviewed Feb 7, 2024

View reviewed changes

a1shadows added 2 commits February 8, 2024 09:25

fixed test case

66afc31

Merge branch 'master' into issue-615/seekToTimestamp

7977687

a1shadows mentioned this pull request Feb 8, 2024

How to continuously keep retrying a step as long as it does not pass? #613

Closed

a1shadows requested a review from authorjapps February 10, 2024 10:01

authorjapps mentioned this pull request Feb 11, 2024

Release with Lombok dependency (with a RC version) #624

Open

authorjapps requested changes Feb 11, 2024

View reviewed changes

removed lombok

822586b

a1shadows requested a review from authorjapps February 13, 2024 02:18

Merge branch 'master' into issue-615/seekToTimestamp

1deb44d

nirmalchandra approved these changes Feb 14, 2024

View reviewed changes

authorjapps approved these changes Feb 14, 2024

View reviewed changes

nirmalchandra merged commit f72319d into authorjapps:master Feb 14, 2024
1 check passed

a1shadows deleted the issue-615/seekToTimestamp branch February 15, 2024 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs #619

ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs #619

a1shadows commented Jan 28, 2024 •

edited

Loading

a1shadows commented Feb 4, 2024

a1shadows commented Feb 7, 2024

authorjapps Feb 7, 2024 •

edited

Loading

a1shadows Feb 7, 2024

authorjapps Feb 7, 2024

a1shadows Feb 9, 2024

authorjapps Feb 9, 2024

a1shadows Feb 11, 2024

authorjapps Feb 11, 2024 •

edited

Loading

a1shadows Feb 13, 2024

authorjapps Feb 14, 2024

a1shadows Feb 14, 2024

authorjapps Feb 7, 2024

authorjapps Feb 9, 2024

authorjapps Feb 11, 2024 •

edited

Loading

nirmalchandra left a comment

ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs #619

ISSUE 615: Changes to support "seekEpoch", "seekTimestamp" in Kafka Consumer step, removing "seek" from ConsumerCommonConfigs #619

Conversation

a1shadows commented Jan 28, 2024 • edited Loading

New Feature: Support to consume on all partitions from specified timestamp/epoch

Motivation and Context

Checklist:

a1shadows commented Feb 4, 2024

a1shadows commented Feb 7, 2024

authorjapps Feb 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

authorjapps Feb 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

authorjapps Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

nirmalchandra left a comment

Choose a reason for hiding this comment

a1shadows commented Jan 28, 2024 •

edited

Loading

authorjapps Feb 7, 2024 •

edited

Loading

authorjapps Feb 11, 2024 •

edited

Loading

authorjapps Feb 11, 2024 •

edited

Loading