Update Grok to use the new Matcher#setTimeout #139405

joegallo · 2025-12-11T20:52:00Z

Follow up to #139075

More recent versions of jruby/joni have the ability to specify a timeout for a Matcher's search (see jruby/joni#78). This is quite a bit easier for us to use than the current scheme of thread registration and monitoring in MatcherWatchdog.Default, and it's faster, too (since there's no synchronization overhead of registering/unregistering).

7c81f85 is just my humble opinion, it's surprising to me that it wasn't added back in #31024, but it wasn't. 🤷

4eabe3e is bug which was introduced by me in #95426. 💀

Personally, I am of the opinion that the MatcherWatchdog interface is entirely obsolete and should be removed. However, there's a fairly intricate third implementation of the interface over in x-pack/plugin/text-structure that I'm not qualified to touch, so I resisted the urge to refactor all the way up through removing the interface completely. As it stands, though, the two first-class default implementations really aren't using the methods of the interface in very meaningful ways anymore, but they still do follow the right shape.

joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep 'implements MatcherWatchdog'
libs/grok/src/main/java/org/elasticsearch/grok/MatcherWatchdog.java:    final class Noop implements MatcherWatchdog {
libs/grok/src/main/java/org/elasticsearch/grok/MatcherWatchdog.java:    final class Default implements MatcherWatchdog {
x-pack/plugin/text-structure/src/main/java/org/elasticsearch/xpack/textstructure/structurefinder/TimeoutChecker.java:    static class TimeoutCheckerWatchdog implements MatcherWatchdog {

Note: the tests compile in this commit, but they do not all pass.

but MatcherWatchdogTests is no longer valuable

elasticsearchmachine · 2025-12-11T20:52:25Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticsearchmachine · 2025-12-11T20:52:25Z

Hi @joegallo, I've created a changelog YAML for you.

dakrone

Thanks for simplifying this, I left some comments but nothing major.

dakrone · 2025-12-11T22:52:34Z

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java

            }
-
+            if (result == Matcher.INTERRUPTED) {
+                throw new RuntimeException(


Should we use an ElasticsearchException here instead, since it's the ES version of RuntimeException?

Grok doesn't have visibility to org.elasticsearch.server, so it can't see that class. We could change that, but I don't want to do that without chatting up a wider audience. I DRYed it in e821c7e to make it just one place and not three, though, so at least the badness is a little more contained.

dakrone · 2025-12-11T22:53:06Z

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java

            matcherWatchdog.unregister(matcher);
        }
+        if (result == Matcher.INTERRUPTED) {
+            throw new RuntimeException(


Same question here for exception.

DRYed in e821c7e.

dakrone · 2025-12-11T22:54:38Z

libs/grok/src/main/java/org/elasticsearch/grok/MatcherWatchdog.java

-        BiConsumer<Long, Runnable> scheduler
-    ) {
-        return new Default(interval, maxExecutionTime, relativeTimeSupplier, scheduler);
+    static MatcherWatchdog newInstance(long maxExecutionTime) {


Since we're touching this anyway, can we changed maxExecutionTime in include the units (millis) in its name? Perhaps maxExecutionMillis?

👍, I went with maxExecutionTimeMillis in 19cc207.

dakrone · 2025-12-11T22:56:36Z

libs/grok/src/main/java/org/elasticsearch/grok/MatcherWatchdog.java

+            if (maxExecutionTime > 0) {
+                matcher.setTimeout(maxExecutionTime * NSEC_PER_MSEC);
+            } else {
+                matcher.setTimeout(-1); // disable timeouts


If we pass in 0 as the time, what is expected by the caller? Is it expected that we would always time out, or that we'd never time out?

Hmmmm, I don't really have a strong opinion about it. I'll poke around for some prior art so that I can claim we're doing something similar to something else common in Elasticsearch.

Do you prefer that 0 means no timeout? ~~I'm fine with just making it do that if that's what you prefer.~~ (edit: wait, that is the current logic)

dakrone · 2025-12-11T22:59:55Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

-        long intervalMillis = IngestSettings.GROK_WATCHDOG_INTERVAL.get(settings).getMillis();
-        long maxExecutionTimeMillis = IngestSettings.GROK_WATCHDOG_INTERVAL.get(settings).getMillis();


As part of this PR, we should mark this setting as deprecated because it no longer does anything, right? Can you update the PR to do that?

Yeah, that's fair. I'll handle that in the morning.

dakrone · 2025-12-11T23:01:51Z

libs/grok/src/main/java/org/elasticsearch/grok/MatcherWatchdog.java

-        private Default(long interval, long maxExecutionTime, LongSupplier relativeTimeSupplier, BiConsumer<Long, Runnable> scheduler) {
-            this.interval = interval;
+
+        private Default(long maxExecutionTime) {


Same here about renaming to include units.

Also covered in 19cc207.

joegallo added 8 commits December 11, 2025 15:36

Fix up some IntelliJ nitpicks

5564b30

These classes can be final

72d4947

Mark that these methods are overrides

d2ee462

Rewrite the Default MatcherWatchdog

00ae709

Note: the tests compile in this commit, but they do not all pass.

Update the newInstance method and callers

d34dd67

Note: the tests compile in this commit, but they do not all pass.

These methods should also check for INTERRUPTED

7c81f85

testExponentialExpressions still shows that this works

5fa7807

but MatcherWatchdogTests is no longer valuable

This should be the GROK_WATCHDOG_MAX_EXECUTION_TIME

4eabe3e

joegallo requested a review from dakrone December 11, 2025 20:52

joegallo added >enhancement :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v9.3.0 labels Dec 11, 2025

Update docs/changelog/139405.yaml

07dbc4e

dakrone approved these changes Dec 11, 2025

View reviewed changes

joegallo added 5 commits December 11, 2025 19:29

Limit the scope of the unchecked suppression

07c540e

Avoid containsKey and then get

8fd99a0

Avoid repeated calls to getBeg

a0996a4

Rename these to maxExecutionTimeMillis

19cc207

DRY the interruption handling

e821c7e

		long intervalMillis = IngestSettings.GROK_WATCHDOG_INTERVAL.get(settings).getMillis();
		long maxExecutionTimeMillis = IngestSettings.GROK_WATCHDOG_INTERVAL.get(settings).getMillis();

Update Grok to use the new Matcher#setTimeout #139405

Are you sure you want to change the base?

Update Grok to use the new Matcher#setTimeout #139405

Uh oh!

Conversation

joegallo commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 11, 2025

Uh oh!

elasticsearchmachine commented Dec 11, 2025

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joegallo Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joegallo commented Dec 11, 2025 •

edited

Loading

joegallo Dec 12, 2025 •

edited

Loading