DeadLetterQueueUtils#extractSegmentId improvement: replace split with index of and substring methods.#18874
DeadLetterQueueUtils#extractSegmentId improvement: replace split with index of and substring methods.#18874mashhurs wants to merge 2 commits intoelastic:mainfrom
Conversation
…ith simple index of and substring methods.
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
| static int extractSegmentId(Path p) { | ||
| return Integer.parseInt(p.getFileName().toString().split("\\.log")[0]); | ||
| final String fileName = p.getFileName().toString(); | ||
| final int dotIndex = fileName.indexOf(".log"); |
There was a problem hiding this comment.
What ensures that we're given a Path p whose filename ends with .log?
If we don't, the dotIndex will be -1, and then we'll get an obscure IndexOutOfBoundsException on the next line when invoking fileName.substring(0, -1).
I'd prefer a capturing regex (e.g., ^([0-9]+)[.]log$), and throwing a clear exception if we don't have a match.
There was a problem hiding this comment.
Right! Initial intention is to just replace the split with indexOf & substring. There is a listFiles (filters files end with .log) safeguard for current DLQ full situation but I generally agree that the method itself isn't safe. I have added lines to intentionally throw exception for the undesired "file doesn't end with .log" case.
|
FYI I've created #18883 to benchmark the various solutions. |
💚 Build Succeeded
History
|
4 participants
Release notes
What does this PR do?
DLQ flush operation is heavy, under the hood
DeadLetterQueueUtils#extractSegmentIdwill be called. TheString#splitis also CPU intensive that internally utilizesPattern#compile. Since the logic is simple, it can be replaceable withindexOfandsubstring.Why is it important/What is the impact to the user?
A bit performance improvement when using DLQ.
Checklist
[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Functionality test can be done by enabling DLQ and changing its settings, such as make size small to make DLQ full.
But performance measurement is a bit hard on powerful machines, so here is the benchmark - #18883
Related issues
Use cases
Screenshots
Logs