-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feature: Add front end for topic events view based on offset range #2614
Conversation
Signed-off-by: khatibtamal <[email protected]>
Signed-off-by: khatibtamal <[email protected]>
Signed-off-by: khatibtamal <[email protected]>
Signed-off-by: khatibtamal <[email protected]>
Hi @khatibtamal - thanks for your PR 🎉 Would you feel comfortable adding this change also to the new UI - the React app in |
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.
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. |
@programmiri just and update. I have made decent progress in Coral and now I am working on fixing the tests, will commit soon. thanks. |
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. |
Signed-off-by: khatibtamal <[email protected]>
@programmiri @muralibasani now this is ready for review. Topic messages can now be fetched using 3 options, they are as follows
|
@programmiri @muralibasani I have had some issues Unintended look at port 9097When 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 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? I ensured there is data in kafka instance running in docker container 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 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 <[email protected]>
Signed-off-by: khatibtamal <[email protected]>
Signed-off-by: khatibtamal <[email protected]>
This is the expected behaviour at the moment, the proxy setup is not perfect yet :D It's described here: 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.
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). |
Also, I just checked the frontend changes in coral and it looks good - great job! Thank you also for adding all needed tests :) |
There was a problem hiding this 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!
@programmiri Thanks a lot for you review! |
@khatibtamal thanks again. I will take a look at it today too. If it looks good, we can merge. |
@khatibtamal UI looks good. If user enters number above or below this range, then warn with a error "Invalid partition id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
sure will add them and make commit soon. thanks! |
Signed-off-by: khatibtamal <[email protected]>
@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. |
There was a problem hiding this 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.
@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. |
Linked issue
Resolves: #2583
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, 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)
main
branch have been pulledpnpm lint
has been run successfully