-
Notifications
You must be signed in to change notification settings - Fork 9
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
Method Throttling #295
Method Throttling #295
Conversation
…ulary terms on term edit." This reverts commit 8c9d086.
…tion event, fix event names, move business vocabulary service test
…description and further throttle testing
… tasks with the same identifier
…on vocabulary persist
…e map to prevent desync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tried, yet so these are purely based on reviewing the code here.
A bunch of inline comments, plus these general remarks:
- Replace JetBrains annotations (particularly
NotNull
) with their Spring equivalents (NonNull
). I stopped pointing that out after a couple of files. We don't useNullable
, it is assumed by default. I also don't see any reason for using these annotations on fields, use them only on public methods - There are several files containing formatting changes only. This makes the already large PR even larger
- I also have a question regarding the necessity of using XML-configured aspect, but let's discuss that on our meeting
src/main/java/cz/cvut/kbss/termit/event/VocabularyContentModifiedEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/cz/cvut/kbss/termit/event/VocabularyCreatedEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/cz/cvut/kbss/termit/event/VocabularyFileTextAnalysisFinishedEvent.java
Outdated
Show resolved
Hide resolved
src/test/java/cz/cvut/kbss/termit/service/document/AnnotationGeneratorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/cz/cvut/kbss/termit/rest/ResourceControllerTest.java
Outdated
Show resolved
Hide resolved
…in throttled context
…rrenceSaver interface and fix faulty throttle aspect test
…is null after transferring out of throttled future
…sage, resolve silenced exception from throttled tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, works nicely. I added a small fix for situations where terms are related to each other, as it was throwing exception due to the analysis of the whole vocabulary running in the same transaction.
…er interface and fix faulty throttle aspect test
…olve silenced exception from throttled tasks
This PR addresses performance issues from #287 and #285
Identified problems
New features
Throttle & Debounce
This PR introduces an option to throttle and debounce method calls.
The goal of method throttling & debouncing is to execute tasks asynchronously on a fixed thread pool merging often method calls into a single task execution with the newest data from the most recent method call [test case]. Throttling ensures that if the method task were not executed in the last X seconds, it would be scheduled for immediate execution [test case]. Otherwise, its execution will be delayed (debounced) so that it can be merged with potential future calls (it guarantees that when no future call comes, the task will be executed with the data from the last call). Task execution also ensures that when a task is time-consuming and its execution is taking longer than the actual throttle interval, a new call to the throttled method won't result in the concurrent execution of the same task [test case].
When a thread is already executing a throttled task, it will ignore any further throttling and will execute all methods synchronously [test case].
Throttling & Debouncing is realized with the Throttle annotation which is handled by the Throttle aspect.
Throttle annotation supports methods with void return type out of the box.
The whole method is, in that case, considered a task that should be throttled, and the method itself will be executed asynchronously.
There is also support for methods returning a Future. However, the concrete returned object MUST be ThrottledFuture. Otherwise the aspect will throw appropriate exception on the method call (there is no way to safely check that on application start).
When a method returns future (ThrottledFuture), the method itself will be executed synchronously allowing to prepare the task that should be throttled and also to provide a cached result which may be acquired by a caller method from CacheableFuture interface before the actual future resolution. The actual task and cached result is then provided through the returned ThrottledFuture object.
An example can be seen in updated result caching validator where the method
validate
will be executed synchronously by the caller thread, checking the cache state and returning already resolved future when the cache is not dirty, or returning the future with the time-consuming taskrunValidation
method and providing the cached result. MethodrunValidation
will be executed asynchronously.Throttled future also implements a chainable future interface, which allows to chain a task that will be executed once the future is resolved.
This, for example, allows the WebSocket controller to respond with the cached result and set a task that will send a new result to the client once new data are available. This prevents the thread from being blocked while awaiting a future resolution.
Scheduling throttled futures also support their cancellation based on their group.
This, for example, allows to cancel scheduling a task to analyze a definition of a single term while an analysis of all terms from the vocabulary is scheduled.
Disadvantages
Long-running tasks
As the application will now run some time-consuming tasks in the background, it will push the status of such tasks to the clients via WebSocket, allowing to display information about the activity to the user.
Currently, it's only possible to name the throttled method by a constant. So, the user will know that there is a validation in progress but won't know which vocabulary is being validated. This might be changed by adding a new parameter for additional information (in addition to the name parameter).
Changes
Requirements and notes
It is expected that TermIt will be deployed in an environment with at least two cores available to benefit from asynchronous processing (more cores would be, of course, beneficial as we need to handle http, websocket, database and background tasks).
The annotation is not prepared for AOT, and a reflection processor registering runtime hints will need to be probably created if the support for AOT is added.