Feature: Add backend for topic events view based on offset range #2551

khatibtamal · 2024-08-15T23:58:36Z

Linked issue

Resolves: #1987

What kind of change does this PR introduce?

What is the current behavior?

Currently it is possible to view only last few selected offsets of partitions.

What is the new behavior?

Backend infrastructure to be able to view topic events based on range.

Other information

This PR will not affect anything in terms of front end usage yet, as a front end PR needs to be created and merged after this.

Requirements (all must be checked before review)

The pull request title follows our guidelines
Tests for the changes have been added (if relevant)
The latest changes from the main branch have been pulled
pnpm lint has been run successfully

muralibasani · 2024-08-19T07:45:39Z

cluster-api/src/main/java/io/aiven/klaw/clusterapi/services/TopicContentsService.java

@@ -72,7 +79,8 @@ public Map<Long, String> readEvents(
    Set<TopicPartition> topicPartitionsSet = consumer.assignment();

    Set<TopicPartition> partitionsAssignment = new HashSet<>();
-    if (offsetPosition.equals(CUSTOM_OFFSET_SELECTION)) {
+    if (offsetPosition.equals(CUSTOM_OFFSET_SELECTION)
+        || offsetPosition.equals(RANGE_OFFSET_SELECTION)) {


In case of RANGE_OFFSET_SELECTION, we should check if start and end offsets are present.

From my testing if the url pattern is not respected then there is a 403, thats why I had to change ClusterApiService.java. So it is impossible that these values will not be present at all. Therefore I made changes to ensure that they are not negative.

muralibasani · 2024-08-19T07:46:16Z

cluster-api/src/main/java/io/aiven/klaw/clusterapi/services/TopicContentsService.java

@@ -18,6 +19,8 @@
 public class TopicContentsService {

  public static final String CUSTOM_OFFSET_SELECTION = "custom";
+  public static final String RANGE_OFFSET_SELECTION = "range";


how abt having an enum for these two custom/range ?

muralibasani · 2024-08-19T07:51:28Z

cluster-api/src/main/java/io/aiven/klaw/clusterapi/controller/TopicContentsController.java

@@ -23,7 +23,7 @@ public class TopicContentsController {
      value =
          "/getTopicContents/{bootstrapServers}/"
              + "{protocol}/{consumerGroupId}/{topicName}/{offsetPosition}/partitionId/{selectedPartitionId}/"
-              + "selectedNumberOfOffsets/{selectedNumberOfOffsets}/{clusterIdentification}",
+              + "selectedNumberOfOffsets/{selectedNumberOfOffsets}/{clusterIdentification}/rangeOffsets/{rangeOffsetsStart}/{rangeOffsetsEnd}",


can offset position be custom or range or id from FE ? so we don't introduce this action in url ?

When I work on FE I will make appropriate changes so that the FE will give offsetPosition as the two existing ones we have (number and custom) plus it will also give range, if thats what the user selects. I preferred to keep this PR only for BE first, then I can work on FE and make another PR.

As far as changes in url, I was a little confused as to how to approach the problem. In current behavior the offsetPosition is used in two cases

offsetPosition can be a number. in this case we ignore the selectedNumberOfOffsets and selectedPartitionId value in the url. Using the offsetPosition number we seek to beginning of topic and pull all data for all partitions upto the given offsetPosition number.

offsetPosition is the string value custom in this case we use the provided values of selectedNumberOfOffsets and selectedPartitionId to only pull data from the provided partition upto the the number provided in selectedNumberOfOffsets

Therefore in my case I felt I had the following options

Reuse the existing url and try to do the range, in this case I have to figure out how to reuse offsetPosition, selectedNumberOfOffsets and selectedPartitionId. If offsetPosition is used to provide the value range then that leaves me with the other two for start and end of range, therefore the data will be for ALL partitions.

Make some changes to url to accommodate the range.

Would you prefer that we keep the existing url exactly same?

How about offsetPosition = { number, custom, range }
Existing situation, if it's number, or custom, nothing changes in the url.

If it's range, we need partitionId, start and end offsets.
Looks good the way you have done it.
"/getTopicContents/{bootstrapServers}/"
+ "{protocol}/{consumerGroupId}/{topicName}/{offsetPosition}/partitionId/{selectedPartitionId}/"
+ "selectedNumberOfOffsets/{selectedNumberOfOffsets}/{clusterIdentification}/"
+ "rangeOffsets/{rangeOffsetsStart}/{rangeOffsetsEnd}",

muralibasani · 2024-08-19T07:54:00Z

cluster-api/src/main/java/io/aiven/klaw/clusterapi/services/TopicContentsService.java

+      for (ConsumerRecord<String, String> record : consumerRecords) {
+        eventMap.put(record.offset(), record.value());
+        if (offsetPosition.equals(RANGE_OFFSET_SELECTION)
+            && (record.offset() >= rangeOffsetsEnd || eventMap.size() >= RANGE_MAX_RECORDS)) {


don't we need range offsets start position ?

I followed the existing pattern and used the rangeOffsetsStart here as we seek to the start position here.

Inside the loop we check the rangeOffsetsEnd only if the selection is range this ensures that the existing default behavior is completely unchanged.

muralibasani

@khatibtamal thanks for the pr. I have not tested yet locally, but have a couple of comments. And how should frontend changes should look like ?

khatibtamal · 2024-08-19T18:48:01Z

@khatibtamal thanks for the pr. I have not tested yet locally, but have a couple of comments. And how should frontend changes should look like ?

@muralibasani thanks for the review. I tested this locally using postman, and created TopicContentsControllerIT.java to thoroughly test all the new and legacy cases. As far as front end, I was hoping to first merge the BE and then do the FE in another PR. My understanding of the architecture for this project is that the FE never directly communicates with cluster-api it communicates via core and the change I made in core for the file ClusterApiService.java should ensure that even if this PR is merged, there should be no impact on FE usage and existing behavior.
Do you prefer the FE to be done in the same PR?

muralibasani · 2024-08-20T07:30:09Z

@khatibtamal thanks for the pr. I have not tested yet locally, but have a couple of comments. And how should frontend changes should look like ?

@muralibasani thanks for the review. I tested this locally using postman, and created TopicContentsControllerIT.java to thoroughly test all the new and legacy cases. As far as front end, I was hoping to first merge the BE and then do the FE in another PR. My understanding of the architecture for this project is that the FE never directly communicates with cluster-api it communicates via core and the change I made in core for the file ClusterApiService.java should ensure that even if this PR is merged, there should be no impact on FE usage and existing behavior. Do you prefer the FE to be done in the same PR?

Hi @khatibtamal it's correct, FE never directly communicates with cluster api.
Let's make the FE changes in a different pr.

muralibasani

Thanks for your comments. Looks good.
After the enum changes, will test locally.

khatibtamal · 2024-08-21T05:05:36Z

@muralibasani pushed changes

Signed-off-by: khatib tamal <[email protected]>

muralibasani · 2024-08-23T13:18:27Z

cluster-api/src/test/java/io/aiven/klaw/clusterapi/TopicContentsControllerIT.java

+public class TopicContentsControllerIT {
+
+  public static final String CUSTOM_SELECTION = "custom";
+  public static final String RANGE_SELECTION = "range";


Can we reuse the enum here ?

muralibasani · 2024-08-23T13:18:34Z

cluster-api/src/test/java/io/aiven/klaw/clusterapi/TopicContentsControllerIT.java

+    topics = {TopicContentsControllerIT.TEST_TOPIC_NAME})
+public class TopicContentsControllerIT {
+
+  public static final String CUSTOM_SELECTION = "custom";


Can we reuse the enum here ?

muralibasani · 2024-08-23T13:59:05Z

cluster-api/src/test/java/io/aiven/klaw/clusterapi/TopicContentsControllerIT.java

+
+  @Test
+  @Order(13)
+  void getTopicContentsWhenRangeAndTotalOffsetsLargerThanMax() throws Exception {


Beautiful tests, I love them.

Signed-off-by: Khatib Tamal <[email protected]>

khatibtamal · 2024-08-24T03:04:23Z

@muralibasani I had to make some changes in ClusterApiController.java in my last commit.

This is because build was failing locally, after investigation I found out it was because of embeddedKafkaBroker which was being initialized in a dynamicPropertySource method, and the broker is not being destroyed after the tests end. This caused a port conflict with newly added tests in TopicContentsControllerIT, as port 9092 was not being shut.

Since EmbeddedKafkaZKBroker is implemented such that the intention is for it to be used as a spring bean (it extends from EmbeddedKafkaBroker which implements DisposableBean and InitializationBean), therefore I removed code for its manual instantiation, and relied on the bean provided by EmbeddedKafka annotation. This way spring will take care of the Initiation and Disposed to ensure the context gets properly created and destroyed for the tests in the class.

muralibasani

Looks good. Approved.

muralibasani · 2024-08-26T07:29:02Z

@muralibasani I had to make some changes in ClusterApiController.java in my last commit.

This is because build was failing locally, after investigation I found out it was because of embeddedKafkaBroker which was being initialized in a dynamicPropertySource method, and the broker is not being destroyed after the tests end. This caused a port conflict with newly added tests in TopicContentsControllerIT, as port 9092 was not being shut.

Since EmbeddedKafkaZKBroker is implemented such that the intention is for it to be used as a spring bean (it extends from EmbeddedKafkaBroker which implements DisposableBean and InitializationBean), therefore I removed code for its manual instantiation, and relied on the bean provided by EmbeddedKafka annotation. This way spring will take care of the Initiation and Disposed to ensure the context gets properly created and destroyed for the tests in the class.

I assume you mean the changes to ClusterApiControllerIT ?

khatibtamal · 2024-08-26T16:45:58Z

ClusterApiController.java

Yes ClusterApiController.java. Sorry about the first link. Thanks for the review. Should I open another issue with the Front End requirements? Or just re open #1987 ? Thanks.

muralibasani · 2024-08-26T16:55:38Z

ClusterApiController.java

Yes ClusterApiController.java. Sorry about the first link. Thanks for the review. Should I open another issue with the Front End requirements? Or just re open #1987 ? Thanks.

Yes pls, now you have the full context. Thanks again.

muralibasani reviewed Aug 19, 2024

View reviewed changes

muralibasani reviewed Aug 20, 2024

View reviewed changes

khatibtamal added 5 commits August 21, 2024 09:22

Feature: Add backend for topic events view based on offset range

1b4238b

Signed-off-by: khatib tamal <[email protected]>

revert accidental change

add2adc

Signed-off-by: khatib tamal <[email protected]>

removed slf4j from TopicContentsControllerIT

a79418b

Signed-off-by: khatib tamal <[email protected]>

code review changes

2bd174a

Signed-off-by: khatib tamal <[email protected]>

ran mvn spotless

44d2450

Signed-off-by: khatib tamal <[email protected]>

muralibasani force-pushed the 1987-view-topic-events-range-backend branch from f8cae7d to 44d2450 Compare August 21, 2024 07:22

muralibasani reviewed Aug 23, 2024

View reviewed changes

khatibtamal added 2 commits August 23, 2024 16:22

Merge branch 'refs/heads/main' into 1987-view-topic-events-range-backend

e7f4c63

code review changes and ClusterApiControllerIT improvement

d385be8

Signed-off-by: Khatib Tamal <[email protected]>

muralibasani approved these changes Aug 26, 2024

View reviewed changes

muralibasani merged commit 10ef3eb into Aiven-Open:main Aug 26, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add backend for topic events view based on offset range #2551

Feature: Add backend for topic events view based on offset range #2551

khatibtamal commented Aug 15, 2024

muralibasani Aug 19, 2024

khatibtamal Aug 21, 2024

muralibasani Aug 19, 2024

khatibtamal Aug 21, 2024

muralibasani Aug 19, 2024 •

edited

Loading

khatibtamal Aug 19, 2024

muralibasani Aug 20, 2024

muralibasani Aug 19, 2024

khatibtamal Aug 19, 2024

muralibasani Aug 20, 2024

muralibasani left a comment

khatibtamal commented Aug 19, 2024

muralibasani commented Aug 20, 2024

muralibasani left a comment

khatibtamal commented Aug 21, 2024

muralibasani Aug 23, 2024

khatibtamal Aug 24, 2024

muralibasani Aug 23, 2024

muralibasani Aug 23, 2024

khatibtamal Aug 23, 2024

khatibtamal commented Aug 24, 2024 •

edited

Loading

muralibasani left a comment

muralibasani commented Aug 26, 2024

khatibtamal commented Aug 26, 2024

muralibasani commented Aug 26, 2024

Feature: Add backend for topic events view based on offset range #2551

Feature: Add backend for topic events view based on offset range #2551

Conversation

khatibtamal commented Aug 15, 2024

Linked issue

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Other information

Requirements (all must be checked before review)

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

muralibasani Aug 19, 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

muralibasani left a comment

Choose a reason for hiding this comment

khatibtamal commented Aug 19, 2024

muralibasani commented Aug 20, 2024

muralibasani left a comment

Choose a reason for hiding this comment

khatibtamal commented Aug 21, 2024

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

khatibtamal commented Aug 24, 2024 • edited Loading

muralibasani left a comment

Choose a reason for hiding this comment

muralibasani commented Aug 26, 2024

khatibtamal commented Aug 26, 2024

muralibasani commented Aug 26, 2024

muralibasani Aug 19, 2024 •

edited

Loading

khatibtamal commented Aug 24, 2024 •

edited

Loading