Added support for timeout millis, refactored unit tests to createObje… #449
Conversation
…ctUnderTest structure Signed-off-by: Taylor Gray <[email protected]>
| recordsOut.add(record); | ||
| } catch (ExecutionException e) { | ||
| LOG.error("An exception occurred while matching on record [{}]", record.getData(), e); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
I'm guessing this came from IntelliJ's auto-complete. We should never be calling printStackTrace(). The line above will print it to the logger just fine.
| } else { | ||
| mergeCaptures(recordMap, grokkedCaptures); | ||
| final Runnable matchTimeoutRunnable = () -> matchAndMerge(recordMap); |
There was a problem hiding this comment.
You should be able to put the lambda directly in runWithTimeout to remove this extra variable. And it is somewhat more common to see lambdas passed into methods.
…ithTimeout Signed-off-by: Taylor Gray <[email protected]>
| } | ||
|
|
||
| @Override | ||
| public boolean isReadyForShutdown() { | ||
| try { | ||
| if (executorService.awaitTermination(grokPrepperConfig.getTimeoutMillis(), TimeUnit.MILLISECONDS)) { |
There was a problem hiding this comment.
Should we couple the grok prepper timeout with the executorService shutdown? How long does this shutdown typically take? Do we want users to be able to control this timeout?
There was a problem hiding this comment.
This is a valid point, especially given that the timeoutMillis can be 0, which would mess up this implementation. What value makes the most sense here? 100 ms? 1 second?
There was a problem hiding this comment.
A couple hundred ms seems fine to me at this time.
|
|
||
| private void runWithTimeout(final Runnable runnable) throws TimeoutException, ExecutionException, InterruptedException { | ||
| Future<?> task = executorService.submit(runnable); | ||
| task.get(grokPrepperConfig.getTimeoutMillis(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
We should update the READMe with this configuration options now that we are using it.
…aitTermination Signed-off-by: Taylor Gray <[email protected]>
| @@ -45,9 +45,13 @@ TODO: provide examples for using each configuration | |||
|
|
|||
| * `target_key` (Optional): A `String` that will wrap all captures for a Record in an additional outer key value. Default value is `null` | |||
|
|
|||
|
|
|||
| * `timeout_millis` (Optional): An `int` that specifies the maximum amount of time, in milliseconds, that matching will be performed on an individual Record before it times out and moves on to the next Record. | |||
| Setting a `timeout_millis = 0` will make it so that matching a Record never times out. Default value is `30,000` | |||
There was a problem hiding this comment.
This could be simplified: "Setting a timeout_millis = 0 will disable this feature. Default value is 30,000"
opensearch-project#449) * Added support for timeout millis, refactored unit tests to createObjectUnderTest structure Signed-off-by: Taylor Gray <[email protected]> * remove redundant e.printStackTrace() and pass lambda directly to runWithTimeout Signed-off-by: Taylor Gray <[email protected]> * Update README to include timeoutmillis, removed timeoutMillis from awaitTermination Signed-off-by: Taylor Gray <[email protected]>
Description
Issues Resolved
Closes #345
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.