Skip to content

Use searchInterruptible for Joni regexp matching#16109

Merged
rschlussel merged 1 commit intoprestodb:masterfrom
kaikalur:regexp_timeout_fix
May 24, 2021
Merged

Use searchInterruptible for Joni regexp matching#16109
rschlussel merged 1 commit intoprestodb:masterfrom
kaikalur:regexp_timeout_fix

Conversation

@kaikalur
Copy link
Contributor

@kaikalur kaikalur commented May 17, 2021

Joni regexp search can be very slow. This can cause runaway splits issue. So we use the searchInterruptible method so it can be interrupted in such cases.

Test plan - None

== RELEASE NOTES ==

General Changes
*  Fix an issue where regular expression functions were not interruptible and could  keep running for a long time after the query they were a part of failed. 

@kaikalur kaikalur force-pushed the regexp_timeout_fix branch from b113bf4 to 9f7f21c Compare May 17, 2021 20:26
@kaikalur kaikalur requested review from aweisberg and rschlussel May 17, 2021 20:27
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

I tested this change from TaskExecutor and it seems to work reasonably well. I did find that it doesn't check interrupt on some regexes.

@kaikalur
Copy link
Contributor Author

I tested this change from TaskExecutor and it seems to work reasonably well. I did find that it doesn't check interrupt on some regexes.

Hmm - can you repro reliably? Maybe some other function handled it already before it came to Joni?

@kaikalur kaikalur requested a review from aweisberg May 17, 2021 21:20
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

It not always detecting interrupt is fine I just noticed that its contract is not guaranteed to throw. This means there are cases where it will execute the regex without ever checking for interrupt.

So for example you could call Thread.interrupt(), then searchInterruptible and it will complete normally.

@aweisberg
Copy link
Contributor

Can you squash (fixup) the additional commit before it gets merged? Thanks!

@kaikalur kaikalur force-pushed the regexp_timeout_fix branch from abf6701 to c8b5adb Compare May 18, 2021 00:23
@kaikalur
Copy link
Contributor Author

Can you squash (fixup) the additional commit before it gets merged? Thanks!

Done

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

See added comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@highker suggested adding Options.DEFAULT directly here instead of passing it in since it is always the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kaikalur kaikalur force-pushed the regexp_timeout_fix branch from c8b5adb to 444dd7a Compare May 20, 2021 15:13
@kaikalur kaikalur force-pushed the regexp_timeout_fix branch from 444dd7a to 20ddbd1 Compare May 20, 2021 18:01
@kaikalur kaikalur requested a review from rongrong May 21, 2021 19:07
@rschlussel
Copy link
Contributor

@kaikalur can you add to the test plan the manual testing you did that demonstrated that this was helpful.

@rschlussel
Copy link
Contributor

rschlussel commented May 24, 2021

also, this should probably get a release note, since it caused production issues.

@aweisberg
Copy link
Contributor

I tested this with #16111 on this query 20210518_201931_00000_y9r9j
In theory this should also help with query timeouts (wall, CPU) and cancellation.

@kaikalur
Copy link
Contributor Author

@kaikalur can you add to the test plan the manual testing you did that demonstrated that this was helpful.

See Ariel's response below. He tested it.

@rschlussel
Copy link
Contributor

Just add a release note, and I'll merge this.

@kaikalur
Copy link
Contributor Author

Just add a release note, and I'll merge this.

Done

@rschlussel rschlussel merged commit e20b4b6 into prestodb:master May 24, 2021
@rschlussel
Copy link
Contributor

I updated the release note to be more focused on the impact of the change. Feel free to fix it if it's not accurate.

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.

3 participants