Skip to content
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

DEV-32671 Grok processing should fail faster on timeout interruption #304

Merged

Conversation

DanMelman
Copy link
Collaborator

@DanMelman DanMelman commented Jul 5, 2022

  1. Add interruption checks before each grok match, to fail processing faster in case of interruption.
  2. Bump org.jruby.joni library version to gain some internal changes that will allow faster interruption of grok matching (jruby/joni@35ddadc)

@DanMelman DanMelman changed the title DEV-32671 GrokProcessor not handling timeout interruption DEV-32671 Grok processing not handling timeout interruption Jul 5, 2022
@DanMelman DanMelman changed the title DEV-32671 Grok processing not handling timeout interruption DEV-32671 Grok processing not handling timeout interruption early enough Jul 5, 2022
@DanMelman DanMelman changed the title DEV-32671 Grok processing not handling timeout interruption early enough DEV-32671 Grok processing should stop faster on timeout interruption Jul 5, 2022
@DanMelman DanMelman changed the title DEV-32671 Grok processing should stop faster on timeout interruption DEV-32671 Grok processing should fail faster on timeout interruption Jul 5, 2022
@alexpalchuk
Copy link
Collaborator

@DanMelman any chance it is reproducible so we can write proper test?

@DanMelman
Copy link
Collaborator Author

@DanMelman any chance it is reproducible so we can write proper test?

There is a test for the interruption itself -testMatchInterrupted.
Is that what you mean @alexpalchuk ?

@alexpalchuk
Copy link
Collaborator

@DanMelman any chance it is reproducible so we can write proper test?

There is a test for the interruption itself -testMatchInterrupted. Is that what you mean @alexpalchuk ?

I mean to have test that fails without fix and passes with fix

Copy link
Contributor

@matvey-mtn matvey-mtn left a comment

Choose a reason for hiding this comment

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

Looking good :)

didn't we want to add Thread.interrupted check here as well?

@DanMelman
Copy link
Collaborator Author

Looking good :)

didn't we want to add Thread.interrupted check here as well?

@matvey-mtn I ended up adding it inside Grok::matches so it is checked on each iteration anyway

@matvey-mtn
Copy link
Contributor

Looking good :)
didn't we want to add Thread.interrupted check here as well?

@matvey-mtn I ended up adding it inside Grok::matches so it is checked on each iteration anyway

ah, ok. i missed it

Comment on lines 96 to 98
if (Thread.currentThread().isInterrupted()) {
throw new InterruptedException();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you have to clear the isInterrupted flag once you inside if

Thread.interrupted()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DanMelman DanMelman requested a review from matvey-mtn July 6, 2022 14:26
Copy link
Contributor

@matvey-mtn matvey-mtn left a comment

Choose a reason for hiding this comment

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

LGTM

I would try to think if there is a way to add tests for really slow Grok pattern match that will cause parser to hang on previous version.

@DanMelman
Copy link
Collaborator Author

@matvey-mtn @alexpalchuk I added the test, it fails on master - 2248651

@DanMelman DanMelman merged commit 1f2369f into master Jul 7, 2022
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.

None yet

3 participants