Async refactor #141

carlosmunoz · 2013-08-23T04:05:48Z

This is a refactor of the background process (now async task) mini-framework inside Zanata. The new changes make the code less verbose and it uses more JVM and Guava constructs like Future and ListenableFuture.

Some things can be expanded upon later, like adding a way to cancel a task even if it means interrupting it. The interface allows for it, but the implementation currently doesn't

… framework.

…read.

Some issues with the original implementation still remain. This revision does not take care of them.

Also change all direct uses of the implementation class to use the interface.

Refactored a few tests to conform to the new async framework.

Add javadoc to new async packages.

davidmason · 2013-08-23T04:29:35Z

zanata-war/src/main/java/org/zanata/action/CopyTransManager.java

@@ -47,6 +45,8 @@
 import lombok.NoArgsConstructor;
 import lombok.Setter;


This file needs imports organized - several unused imports here.

I don't see any unused imports in this file. Are you sure you commented on the right one?

Definitely looks like the right file to me. Eclipse is giving me unused import warnings for:

import com.google.common.collect.MapMaker; ... import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.concurrent.TimeUnit;

I think you're right. For some reason Idea was not showing me the unused imports.

davidmason · 2013-08-26T00:57:52Z

zanata-war/src/main/java/org/zanata/service/AsyncTaskManagerService.java

+{
+   <V, H extends AsyncTaskHandle<V>> String startTask(AsyncTask<V, H> task);
+
+   <V, H extends AsyncTaskHandle<V>> void startTask(AsyncTask<V, H> task, Serializable key);


When would I start a task with a key vs. without a key? I think some javadoc would be handy to provide the information.

Only for Document and Iteration so far, but could be extended to others i nthe future.

davidmason · 2013-08-26T04:11:13Z

zanata-war/src/main/java/org/zanata/async/AsynchronousTaskExecutor.java

 @Scope(ScopeType.STATELESS)
 @AutoCreate
 @Slf4j
-public class AsynchronousExecutor
+public class AsynchronousTaskExecutor


Since this class is only intended for use through TaskExecutor, it could be made package-protected to reduce the possibility of inappropriate use.

It can't be package protected because Seam needs component classes to be public. And this needs to be a component class in order to take advantage of the @Asynchronous annotation. That's the reason for adding the comment as well.

Sounds reasonable.

What is the reason not to use it directly? Might be handy to mention that in the javadoc.

It's because we need an executor that is able to return the handle
after starting the process in the background. Methods marked as
asynchronous should not return anything, so we need that extra level of
nesting to execute some things synchronously (like returning the
handle) and others asynchronously (the task itself). Maybe in the
future we can integrate this class more closely with
java.util.concurrent.ExecutorService, but for the time being it's not
really needed. I'll add some brief javadoc for this.

Carlos A. Munoz
Senior Software Engineer
Engineering - Internationalization
Red Hat

On Mon 26 Aug 2013 03:33:26 PM EST, David Mason wrote:

In
zanata-war/src/main/java/org/zanata/async/AsynchronousTaskExecutor.java:

@scope(ScopeType.STATELESS)
@autocreate
@slf4j
-public class AsynchronousExecutor
+public class AsynchronousTaskExecutor

Sounds reasonable.

What is the reason not to use it directly? Might be handy to mention
that in the javadoc.

—
Reply to this email directly or view it on GitHub
https://github.com/zanata/zanata-server/pull/141/files#r5971423.

davidmason · 2013-08-26T07:20:22Z

zanata-war/src/main/java/org/zanata/async/tasks/ZipFileBuildTask.java

+   }
+
+   @Override
+   public String call() throws Exception


This method could probably be split into several smaller methods for readability - not a blocker for this merge, but worth noting somewhere.

davidmason · 2013-08-26T08:47:31Z

It looks like there are several places where async tasks can be running but might not have a process handle available. Is this expected much under normal circumstances? If it isn't normal, we could probably simplify a lot of code by failing early if a handle isn't available.

davidmason · 2013-08-26T08:56:14Z

zanata-war/src/main/java/org/zanata/webtrans/server/rpc/DownloadAllFilesHandler.java

         {
            // Cancel any other processes
-            this.zipFilePrepHandle.stop();
+            this.zipFilePrepHandle.cancel(false);


cancel(false) is a little confusing at call sites like this. We can make our AsyncTaskHandle friendlier than AbstractFuture by adding a methods like:

public boolean cancelIfNotStarted() { return cancel(false); } public boolean forceCancel() { return cancel(true); }

Not sure abut this one. First, the boolean passed to the method is indicating whether the thread running the task may be interrupted while running or not (cancel(false) doesn't really mean cancelling only if not started; some tasks understand the concept of being cancelled and will stop processing when this signal is received). Currently we don't support that thread interruption, so the boolean is meaningless really. But I'd like to leave the option open to implement such a feature, where we are able to asynchronously run tasks that were not meant to be asynchronous (an thus have no concept of "stopping"), but at the same time being able to forcefully cancel them. this might require some thought and prototyping (and might not even be possible with Seam 2), and since it wasn't really an option in the previous setup, I didn't bother going into it.

My comment is just about readability at the call site - when someone reads a line that says:

handle.cancel(false);

they can only guess at what that might mean (and will likely guess wrong) - basically it forces them to look up the javadoc or find the implementation, which is disruptive to quickly getting an understanding of what the code does.

My suggestion here is just to wrap the parameterized method in a couple of more readable methods, so that all code that cancels an async process is more readable, so readers don't have to do a double-take and investigate when they're just trying to get a quick overview of what the code is doing, e.g. you hit a line that says:

handle.cancel()

or

handle.forceCancel()

or something else that makes sense right away.

I still have mixed feelings about this. One the one hand the methods are more specific, but on the other now we have three methods that do a cancel operation, most of them being logically equivalent. So, if avoiding a javadoc lookup is what you're after, you've just traded the lookup when trying to understand the code, for a lookup when trying to actually cancel an operation (and deciding which of the three cancel methods you want to use).

But it's probably not worth dragging this discussion any longer, so I'll cave and just add the methods :)

Ah yes, ideally the method that takes the boolean would not be there, but that's not how Java does things. Better still would be if we could do:

handle.cancel(interruptIfRunning=true)

but this isn't python.

Other minor name and documentation changes.

carlosmunoz · 2013-08-26T23:50:19Z

Not sure what you mean here. The whole idea is structured so that
running an async task gives you a handle always. You may choose to
ignore the handle, or even the task implementation may ignore the handle
(there's nothing really to prevent you from doing that), but not sure
which places you are talking about where async tasks are running without
a process handle being available.

Carlos A. Munoz
Senior Software Engineer
Engineering - Internationalization
Red Hat

On 08/26/2013 06:47 PM, David Mason wrote:

It looks like there are several places where async tasks can be running
but might not have a process handle available. Is this expected much
under normal circumstances? If it isn't normal, we could probably
simplify a lot of code by failing early if a handle isn't available.

—
Reply to this email directly or view it on GitHub
#141 (comment).

davidmason · 2013-08-27T00:21:32Z

zanata-war/src/main/java/org/zanata/service/impl/CopyTransServiceImpl.java

+   public void copyTransForDocument(HDocument document, HCopyTransOptions copyTransOpts)
+   {
+      Optional<CopyTransTaskHandle> asynchronousProcessHandle =
+            AsyncUtils.getEventAsyncHandle(CopyTransTaskHandle.class);


Eclipse is saying this is unused.

davidmason · 2013-08-27T00:30:27Z

@carlosmunoz actually a closer look at the optional process handles shows that there are only a few of them in CopyTransServiceImpl, where it looks up the handle using AsyncUtils and will increment progress or similar only if the handle is present. It seems odd that there would not be a handle for the running process, so I was wondering whether we would ever expect not to have a handle. I have a feeling it isn't particularly important.

carlosmunoz · 2013-08-27T00:46:33Z

@davidmason The ideal (though admittedly probably not possible) would be to have any method being able to be called asynchronously or synchronously, with progress updates happening automatically. The best we have at the moment is to update progress optionally, so that nothing breaks when calling synchronously or asynchronously.

We would expect not to have a handle when the process is called synchronously, and that is why AsyncUtils returns an Optional. We can find better ways of doing this, but I just had to draw a line on how much to implement for this pull request.

davidmason · 2013-08-27T00:57:37Z

@carlosmunoz ok, that makes sense if it can be run synchronously as well.

For any operation that can report progress, reports could just be ignored when it is running synchronously. As for automatic progress updates, I think that will only ever be possible in cases where we can reasonably define a set of tasks that the async task runner could iterate over and run each - probably impossible for some operations and certainly too much to do any time soon.

Still not sure about this duplication of cancel methods, but lets see how it goes.

…nal now. Initally though about throwing an exception, but really the estimated time might be asked for even after the process has started, so it's not really suitable to throw an exception in that case. Returning an optional seems like the next best possibility.

…efactor Conflicts: zanata-war/src/main/java/org/zanata/action/TranslationMemoryAction.java zanata-war/src/main/java/org/zanata/service/ProcessManagerService.java zanata-war/src/main/java/org/zanata/service/impl/ProcessManagerServiceImpl.java

Make sure the translation memory uses of the async classes is working properly.

davidmason · 2013-08-27T06:21:09Z

👍

Async refactor

Carlos A. Munoz and others added 11 commits August 16, 2013 17:30

New artifacts for refactored async processes.

8a2eec1

First attempt at migrating the Zip file process to the new Async task…

b4860c4

… framework.

Add feature for async tasks to inherit the identity of the calling th…

cb2511d

…read.

Move copy trans to the new Async task framework.

7554ca8

Migrate the Asynchronous Rest service to the new task framework.

f323076

Refactor the reindex process to use the new async task framework.

e61464e

Some issues with the original implementation still remain. This revision does not take care of them.

Rename AsyncHandle to AsyncTaskHandle.

1a3e26d

Extract interface for new AsyncTaskManagerService class.

0325ea1

Also change all direct uses of the implementation class to use the interface.

Completely migrate remaining users to the new Async task framework.

8dc9549

Refactored a few tests to conform to the new async framework.

Rewire Seam's Contexts static methods for Seam Autowire.

b60ceb7

Add javadoc to new async packages.

Finish asynchronous task integration tests.

88d8ef3

ghost assigned davidmason Aug 23, 2013

davidmason reviewed Aug 23, 2013
View reviewed changes

Carlos A. Munoz added 3 commits August 26, 2013 09:43

Organize imports on Copy Trans Manager.

0e786ff

Rename async handle 'time' methods.

073e7ff

Removing sorting by 'toString' i nthe Process manager action.

eace4aa

davidmason reviewed Aug 26, 2013
View reviewed changes

Create different CopyTransTask implementations for different targets.

a5806f3

Only for Document and Iteration so far, but could be extended to others i nthe future.

davidmason reviewed Aug 26, 2013
View reviewed changes

Carlos A. Munoz added 2 commits August 27, 2013 09:35

Change async process key generating algorithm to not reuse keys.

0981b7d

Other minor name and documentation changes.

Remove no-longer-used private class.

aa25dc0

davidmason reviewed Aug 27, 2013
View reviewed changes

Remove unused variable.

42908c4

Carlos A. Munoz added 5 commits August 27, 2013 11:45

Add cancel methods to the AsyncTaskHandle.

edff342

Still not sure about this duplication of cancel methods, but lets see how it goes.

Refactor methods.

bd7ab16

Fix manual merge compilation errors.

af938bc

Make sure the translation memory uses of the async classes is working properly.

davidmason added a commit that referenced this pull request Aug 27, 2013

Merge pull request #141 from zanata/async-refactor

e875eea

Async refactor

davidmason merged commit e875eea into integration/master Aug 27, 2013

davidmason deleted the async-refactor branch August 27, 2013 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async refactor #141

Async refactor #141

carlosmunoz commented Aug 23, 2013

davidmason Aug 23, 2013

carlosmunoz Aug 24, 2013

davidmason Aug 25, 2013

carlosmunoz Aug 25, 2013

davidmason Aug 26, 2013

davidmason Aug 26, 2013

carlosmunoz Aug 26, 2013

davidmason Aug 26, 2013

carlosmunoz Aug 26, 2013

davidmason Aug 26, 2013

davidmason commented Aug 26, 2013

davidmason Aug 26, 2013

carlosmunoz Aug 26, 2013

davidmason Aug 27, 2013

carlosmunoz Aug 27, 2013

davidmason Aug 27, 2013

carlosmunoz commented Aug 26, 2013

davidmason Aug 27, 2013

davidmason commented Aug 27, 2013

carlosmunoz commented Aug 27, 2013

davidmason commented Aug 27, 2013

davidmason commented Aug 27, 2013

		@@ -47,6 +45,8 @@
		import lombok.NoArgsConstructor;
		import lombok.Setter;

Async refactor #141

Async refactor #141

Conversation

carlosmunoz commented Aug 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmason commented Aug 26, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosmunoz commented Aug 26, 2013

Choose a reason for hiding this comment

davidmason commented Aug 27, 2013

carlosmunoz commented Aug 27, 2013

davidmason commented Aug 27, 2013

davidmason commented Aug 27, 2013