MultiLineBuildLogIndication performance and bug fixes #59
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since inherited classes from Indication does not declare @JsonProperty("pattern") in their constructors, the JSON writer will use the default constructor and populate fields using a matching (by name) getter. This leads to one particular bad bug, where MultiLineBuildLogIndication returns a modified pattern using the @Overidden getPattern() during JSON serialization. Everytime a user modify a FailureCause containing a MultiLineBuildLogIndication it will be rewritten when stored in mongodb: 1. User enters a MultiLineBuildLogIndication: email.com 2. This is stored: (?m)(?s)^[\r\n]*?email.com[^\r\n]*?$ 3. This is searched for in Logs: (?m)(?s)^[\r\n]*?(?m)(?s)^[\r\n]*?email.com[^\r\n]*?$[^\r\n]*?$ 4. Next time the Cause is saved: (?m)(?s)^[\r\n]*?(?m)(?s)^[\r\n]*?(?m)(?s)^[\r\n]*?email.com [^\r\n]*?$[^\r\n]*?$[^\r\n]*?$ Rinse and repeat. Change-Id: Id1233b66d29aa948813a29bcc01f353c4d245304
Test data --------- Log: 49MB log @ 280.000 lines Multiline pattern: Simple pattern, matching +20 lines at end of log Pre-fix: 32 seconds (Note that the hardcoded timeout is 10 seconds) Post-fix: 1.7 seconds The fix ------- Current implementation relies on Scanner.findWithinHorizon() with a horizon of 10k. Every time this large window does not turn up a match, the sliding window increases just one line (or several if consecutive empty lines are found). This essentially means that every line in a huge log is scanned 100s of times. To avoid grabbing extremely large captures, we now scan the log in 10kb chunks with 5kb overlaps (~50 lines for regular logs). This patch also fixes an issue where the multiline pattern always needed to match the beginning of a line. Change-Id: I5eeddb3555f965dde463015766f1800c5bcb772b
GLundh
changed the title
|
At least one test failure seems legit, rebuilding since some of the tests where due to timezone issues. |
|
Yepp, the three remaining test failures seems legit. |
One testcase was a bit broken (it only worked due to a somewhat faulty compiled pattern in previous versions of MultilineBuildLogIndication). This test case was replaced with a more stable one that utilizes the block timeout. QuadrupleDupleLineReader() is not used for Multiline scanning since it does not use Reader.readline(). Another testcase utilized a deprecated schedule method and did not receive Causes due to this. Change-Id: I6cf0dfc81869aad965c3d2731cfb8f9121da584f
|
Thanks for looking at the code Bobby. I have updated the testcases. |
|
Hang on. Just noticed the checkstyle. |
Change-Id: I8f633d61f0abc0b2ef98adb33c9799c87e9b3436
|
It's hard to tell; does this change require a minor version bump? Or is the behaviour consistent enough with previous versions to just bump the micro version? |
|
Thanks!
Hard to say indeed. Multiline + Mongo is broken today. So that fix should go in as soon as possible. The performance stuff should be drop in replacement. But looking at the release history, performance fixes have motivated major releases in the past (see 1.15 -> 1.16). Or perhaps it was the xss-fix that motivated that version jump? |
|
Yes, it was the size of the xss fix that prompted the minor bump instead of a micro bump. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fork provides some needed MultiLineBuildLogIndication
performance and bug fixes (see commit messages for more details):
JSON Serialization
Since inherited classes from Indication does not declare
@JsonProperty("pattern") in their constructors, the JSON
writer will use the default constructor and populate fields
using a matching (by name) getter.
This leads to one particular bad bug, where
MultiLineBuildLogIndication returns a faulty modified pattern
using the @Overidden getPattern() during JSON serialization.
Performance fixes.
Test data:
Log: 49MB log @ 280.000 lines
Multiline pattern: Simple pattern, matching +20 lines at end of log
Pre-fix: 32 seconds (Note that the hardcoded timeout is 10 seconds)
Post-fix: 1.7 seconds
Multiline patterns could only match beginning of a line