Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add front end for topic events view based on offset range #2614

Merged

Conversation

khatibtamal
Copy link
Contributor

@khatibtamal khatibtamal commented Sep 9, 2024

Linked issue

Resolves: #2583

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Docs update
  • CI update

What is the current behavior?

Currently it is possible to view only last few selected offsets of partitions, the backend was added in #1987 .

What is the new behavior?

Now topic events can be views based on offset range and partition.

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

Signed-off-by: khatibtamal <khatibtamal@gmail.com>
Signed-off-by: khatibtamal <khatibtamal@gmail.com>
Signed-off-by: khatibtamal <khatibtamal@gmail.com>
@khatibtamal
Copy link
Contributor Author

khatibtamal commented Sep 9, 2024

Before
image

After
image

Before
image

After
image

New Feature
image

@programmiri
Copy link
Contributor

Hi @khatibtamal - thanks for your PR 🎉

Would you feel comfortable adding this change also to the new UI - the React app in /coral?

@khatibtamal
Copy link
Contributor Author

Hi @khatibtamal - thanks for your PR 🎉

Would you feel comfortable adding this change also to the new UI - the React app in /coral?

Hi @programmiri I will push the changes in a day or two, just need some time getting used to the coral code thanks.

@programmiri
Copy link
Contributor

programmiri commented Sep 11, 2024

Hi @programmiri I will push the changes in a day or two, just need some time getting used to the coral code thanks.

Thanks for the update - and take your time!

We've tried to document the processes and approaches in coral a lot, I hope the docs can support you 🤞 Please feel free to ping me if you have any question! And just to be clear - it's also ok if you don't want to do that, we can add a follow up PR later. We appreciate the work you've done already.

@khatibtamal
Copy link
Contributor Author

khatibtamal commented Sep 12, 2024

Hi @programmiri I will push the changes in a day or two, just need some time getting used to the coral code thanks.

Thanks for the update - and take your time!

We've tried to document the processes and approaches in coral a lot, I hope the docs can support you 🤞 Please feel free to ping me if you have any question! And just to be clear - it's also ok if you don't want to do that, we can add a follow up PR later. We appreciate the work you've done already.

Hi, the documentation was excellent, I only had a couple of minor issues in running the new UI. They are as follows.

  1. Step 9 as listed here did not create the zookeeper and kafka containers for me. I manually ran docker-compose-testEnv.yaml for those containers.
  2. Step 12 first bullet here states kafka bootstrap server is http://klaw-kafka:9092 but while adding the cluster in klaw, just need to ensure the protocol http is skipped, and I needed to put exactly klaw-kafka:9092 even localhost:9092 and 127.0.0.1:9092 did not work for me.

I have coral working locally now and I am able to make changes. I dont mind attempting this, I feel within a day or two I should be able to put up a PR. I will certainly ping you if I feel the issue gets too hot to handle.
thanks.

@khatibtamal
Copy link
Contributor Author

@programmiri just and update. I have made decent progress in Coral and now I am working on fixing the tests, will commit soon. thanks.

@programmiri
Copy link
Contributor

Hi @kathia-barahona thanks so much for your feedback, I'll use that to update our documentation, that was super valuable! ❤️ Will take a look at your PR later (probably tomorrow)

@khatibtamal
Copy link
Contributor Author

Hi @kathia-barahona thanks so much for your feedback, I'll use that to update our documentation, that was super valuable! ❤️ Will take a look at your PR later (probably tomorrow)

Cool thanks! I will push my changes with all tests in a day or two.

@khatibtamal
Copy link
Contributor Author

khatibtamal commented Sep 19, 2024

@programmiri @muralibasani now this is ready for review.

Topic messages can now be fetched using 3 options, they are as follows

  1. Default: Behaviour exactly same as before
image
  1. Custom: Behaviour exactly same as before
image
  1. Range: this is the new behaviour, where we can now give an offsetStart and offsetEnd for a partitionId
image

@khatibtamal
Copy link
Contributor Author

khatibtamal commented Sep 19, 2024

@programmiri @muralibasani I have had some issues

Unintended look at port 9097

When I first login for some reason I am directed to a localhost:9097/coral path, and my changes do not give an intended look as follows
image

But when I change the port to 1337 then it looks fine. Not sure why this is happening, is there anything else I need to do in the code to avoid this?

Valid urls give no messages (even with legacy code)

When I press the "Fetch messages" button, I never get any data back, even though I ensured there is data in the kafka instance running in the docker container. This also happened to me using legacy code, with no changes, maybe I missed something during setup?

No data given in UI
image

I ensured there is data in kafka instance running in docker container
image

My logs in the cluster-api instance running in docker, shows that it seems that the request is flowing correctly from coral->core->clusterApi but clusterApi is not being able to pull data from kafka
image

There is no problem if the entire setup is run without docker (core, clusterApi, kafka, zookeeper)

When I run the complete application without docker containers I have no problem in fetching data using the legacy UI, but I cannot run coral that way. So I could not test end to end using Coral.

Any recommendations?

Given the above mentioned issues, is there is anything I can do or any code I need to fix on my end?

Signed-off-by: khatibtamal <khatibtamal@gmail.com>
Signed-off-by: khatibtamal <khatibtamal@gmail.com>
Signed-off-by: khatibtamal <khatibtamal@gmail.com>
@programmiri
Copy link
Contributor

But when I change the port to 1337 then it looks fine. Not sure why this is happening, is there anything else I need to do in the code to avoid this?

This is the expected behaviour at the moment, the proxy setup is not perfect yet :D It's described here:
https://github.com/Aiven-Open/klaw/tree/main/coral/proxy#installation

At the moment, if you're not logged in as a user (or the authentication expired), the redirect will go to port 9097 and not port 1337, so in this moment you're looking at Klaw is it runs in the docker container and not at the dev server. So after login, we've to manually change to the correct port (1337) again.

When I press the "Fetch messages" button, I never get any data back, even though I ensured there is data in the kafka instance running in the docker container. This also happened to me using legacy code, with no changes, maybe I missed something during setup?
When I run the complete application without docker containers I have no problem in fetching data using the legacy UI, but I cannot run coral that way. So I could not test end to end using Coral.

That's a great input! I didn't notice that, but I also don't have any topics with messages on my local env at the moment. I will tag @aindriu-aiven and @muralibasani here to see who can take a look so we can make sure it's only an issue with the proxy (and maybe see how we can fix it).

@programmiri
Copy link
Contributor

Also, I just checked the frontend changes in coral and it looks good - great job! Thank you also for adding all needed tests :)

programmiri
programmiri previously approved these changes Sep 23, 2024
Copy link
Contributor

@programmiri programmiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for changes in /coral, looks great!

@khatibtamal
Copy link
Contributor Author

👍 for changes in /coral, looks great!

@programmiri Thanks a lot for you review!

@muralibasani
Copy link
Contributor

@khatibtamal thanks again. I will take a look at it today too. If it looks good, we can merge.

@muralibasani
Copy link
Contributor

@khatibtamal UI looks good.
Can we add a small validation on the partition id field. Based on the number of partitions, we should not allow the user to enter any other number.
If topic has 2 partitions for ex: then partition ids are 0 and 1.
If topic has 10 partitions for ex: then partition ids are 0 , 1, 2....9

If user enters number above or below this range, then warn with a error "Invalid partition id"

Copy link
Contributor

@muralibasani muralibasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@khatibtamal
Copy link
Contributor Author

@khatibtamal UI looks good. Can we add a small validation on the partition id field. Based on the number of partitions, we should not allow the user to enter any other number. If topic has 2 partitions for ex: then partition ids are 0 and 1. If topic has 10 partitions for ex: then partition ids are 0 , 1, 2....9

If user enters number above or below this range, then warn with a error "Invalid partition id"

sure will add them and make commit soon. thanks!

Signed-off-by: khatibtamal <khatibtamal@gmail.com>
@khatibtamal
Copy link
Contributor Author

khatibtamal commented Oct 3, 2024

@khatibtamal UI looks good. Can we add a small validation on the partition id field. Based on the number of partitions, we should not allow the user to enter any other number. If topic has 2 partitions for ex: then partition ids are 0 and 1. If topic has 10 partitions for ex: then partition ids are 0 , 1, 2....9

If user enters number above or below this range, then warn with a error "Invalid partition id"

@muralibasani @programmiri pushed changes, my last commit.

@muralibasani in this comment I have described how the back end when using coral does not return any data, even though I ensured there is data, did you have the same issue?

@muralibasani
Copy link
Contributor

@khatibtamal UI looks good. Can we add a small validation on the partition id field. Based on the number of partitions, we should not allow the user to enter any other number. If topic has 2 partitions for ex: then partition ids are 0 and 1. If topic has 10 partitions for ex: then partition ids are 0 , 1, 2....9
If user enters number above or below this range, then warn with a error "Invalid partition id"

@muralibasani @programmiri pushed changes, my last commit.

@muralibasani in this comment I have described how the back end when using coral does not return any data, even though I ensured there is data, did you have the same issue?

Thank you for the changes @khatibtamal Will take a look very soon.

Copy link
Contributor

@muralibasani muralibasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khatibtamal changes look good. Big thanks for this submission.

@khatibtamal
Copy link
Contributor Author

@khatibtamal changes look good. Big thanks for this submission.

@muralibasani thanks for your review. Also do you have similar issues on your local when using Coral proxy, as mentioned in the second point Valid urls give no messages in this comment?

@muralibasani muralibasani merged commit b6c8f05 into Aiven-Open:main Oct 7, 2024
30 checks passed
@muralibasani
Copy link
Contributor

@khatibtamal changes look good. Big thanks for this submission.

@muralibasani thanks for your review. Also do you have similar issues on your local when using Coral proxy, as mentioned in the second point Valid urls give no messages in this comment?

@khatibtamal will take a look at the issue soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Front End for Option to view topic events based on offset range for a partition
3 participants