-
Notifications
You must be signed in to change notification settings - Fork 848
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
Add Message Search Functionality #288
Conversation
…eading the entire topic into memory and having a safetly limit on the number of messages to read befor the search aborts. Added a maximum results count. Added a date filter to allow user to specify which timestamp to start searching from. Added better output.
@nathanscottdaniels |
That's not how pull requests work. Only obsidian dynamics can merge it, at their discretion. If you want to use this feature, you can clone my fork and build it locally. |
Hi Obsidian Dynamics - any thoughts on merging this PR? I'm looking for a Kafka tool that can do message searching within topics, and kafdrop with this PR looks ideal! |
How search is implemented? Anyway for sure conflicts must be resolved before we can start evaluating it. |
The code in this PR as well as its description answer all of your questions and concerns. But to summarize, searching is done within kafdrop by seeking to a point in time in the kafka topic and reading messages, filtering out those that don't match the search. I have put various safety measures in place to ensure the kafaka brokers are not abused, and this functionality is no more or less intensive the the brokers than a user of kafdrop flipping through pages of message previews. |
I do not have a working Java development environment anymore so another volunteer will need to resolve the merge conflicts, sorry. |
I will take care of that |
@nathanscottdaniels Can you allow me to push to the |
@Bert-R I think you can just fork his branch and add more commits, then create another PR. This should maintain history and credits... |
Just sent you an invite |
Refactored to increase maintainability
} else if (scanStatus.scannedCount() >= SEARCH_MAX_MESSAGES_TO_SCAN) { | ||
completionReason = CompletionReason.EXCEEDED_MAX_SCAN_COUNT; | ||
} else { | ||
// TODO: This situation cannot occur. There is no exit criterion on end of timespan |
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.
@nathanscottdaniels See this TODO. At the moment, there is no way the search ends on reaching the end of the timespan. If it would, it would result in the below message to the user:
Search reached the end of the specified time span before finding requested number of results. Scanned %d messages.
How do you want to resolve this? Add the search criterion or drop the end state? The latter, I can do for you. For the former, I'd have to depend on you.
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.
I guess I missed that. Can you just drop that end state? I appreciate your help.
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.
@nathanscottdaniels Done
@davideicardi It's compiling again and I've refactored it a bit for maintainability. One update is needed, based on a question I asked to Nathan. After that, I feel it's OK to merge this. The search behavior is just based on regular seeking and polling. I don't think it'll cause issues with the broker. |
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.
Looks good now
@nathanscottdaniels Thanks for the contribution!
@davideicardi Do you want to take a look? Otherwise it can be merged.
This change will bump the version of the kafdrop image. It includes numerous Dependency updates as well as bugfixes, and security patching. Also includes some shiny new features such as: - Added Message Search functionality [#288](obsidiandynamics/kafdrop#288) - Enabled dependabot to update deps and base image tag [#404](obsidiandynamics/kafdrop#404) **Breaking Changes**: - Update to JDK 17 and SpringBoot 3 in upstream [#482](obsidiandynamics/kafdrop#482) For the complete list of changes see: [Releases](https://github.com/obsidiandynamics/kafdrop/releases) [Comparing Changes](obsidiandynamics/kafdrop@3.30.0...4.0.1)
This change will bump the version of the kafdrop image. It includes numerous Dependency updates as well as bugfixes, and security patching. Also includes some shiny new features such as: - Added Message Search functionality [#288](obsidiandynamics/kafdrop#288) - Enabled dependabot to update deps and base image tag [#404](obsidiandynamics/kafdrop#404) **Breaking Changes**: - Update to JDK 17 and SpringBoot 3 in upstream [#482](obsidiandynamics/kafdrop#482) For the complete list of changes see: [Releases](https://github.com/obsidiandynamics/kafdrop/releases) [Comparing Changes](obsidiandynamics/kafdrop@3.30.0...4.0.1)
Overhaul search functionality prototype first introduced by @plmarcelo in PR #175.
Improved performance by not reading the entire topic into memory and having a safety limit on the number of messages to read before the search aborts. Added a maximum results count. Added a date filter to allow user to specify which timestamp to start searching from. Added better output.