MNG-3004 review - (Experimental) multithreading support

[Kristian Rosenvold <kr...@gmail.com>]

I have reviewed the changeset made in the MNG-3004 branch. I don't know
if the community will accept my review, but I'll make a stab at it
anyway. 

I think the implementation overall looks really good. My only real
question is about the session cloning and session merging. What's the
deal with that ?

I have worked some more on the basic implementation of the code. I did
not change anything significant, but I improved the testability
significantly and I also wrote a quite comprehensive unit test.

(My patch basically creates a stronger separation between concurrency
dependency analysis and task execution. The dependency analysis
has been separated into a separate class call ConcurrencyDependencyGraph
with its own unit-test, ConcurrencyDependencyGraphTest.)

As for further perspectives, I see that the current implementation
does not advance-load external dependencies for non-schedulable builds.
This certainly seems like a nice future possibility, but I think it is
good judgment to leave this for a future version; this stuff is going to
create enough stir ;) This implementation is clean and does not close
this option for the future in any way.

Compatibility with non-threaded version seems to be retained nicely by
simply using a (mostly) separate branch of execution, with minimal
code duplication.

In one of the patches I supplied on MNG-3004 I also resurrected the 
StringSearchModelInterpolatorTest from the 2.2.X branch, and added
a failing test related to demonstrating the concurrency fix I
reported last week. It was not immediately clear/obvious to me
if all of the tests in that base class AbstractModelInterpolatorTest
were still relevant, and someone has to take a look at them. They were
failing.

There is is still the issue of handling snapshot artifacts,
but the patch has been tested on a highly concurrent build
server for some time now, and seems stable for a build
without any snapshot artifacts.

Regards,

Kristian Rosenvold



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

Re: MNG-3004 review - (Experimental) multithreading support

[Jason van Zyl <ja...@maven.org>]

I'm fried at the moment, but I'll take a look tomorrow night and try  
to comment on Wednesday.

On 2009-11-16, at 11:10 PM, Kristian Rosenvold wrote:

> I have reviewed the changeset made in the MNG-3004 branch. I don't  
> know
> if the community will accept my review, but I'll make a stab at it
> anyway.
>
> I think the implementation overall looks really good. My only real
> question is about the session cloning and session merging. What's the
> deal with that ?
>
> I have worked some more on the basic implementation of the code. I did
> not change anything significant, but I improved the testability
> significantly and I also wrote a quite comprehensive unit test.
>
> (My patch basically creates a stronger separation between concurrency
> dependency analysis and task execution. The dependency analysis
> has been separated into a separate class call  
> ConcurrencyDependencyGraph
> with its own unit-test, ConcurrencyDependencyGraphTest.)
>
> As for further perspectives, I see that the current implementation
> does not advance-load external dependencies for non-schedulable  
> builds.
> This certainly seems like a nice future possibility, but I think it is
> good judgment to leave this for a future version; this stuff is  
> going to
> create enough stir ;) This implementation is clean and does not close
> this option for the future in any way.
>
> Compatibility with non-threaded version seems to be retained nicely by
> simply using a (mostly) separate branch of execution, with minimal
> code duplication.
>
> In one of the patches I supplied on MNG-3004 I also resurrected the
> StringSearchModelInterpolatorTest from the 2.2.X branch, and added
> a failing test related to demonstrating the concurrency fix I
> reported last week. It was not immediately clear/obvious to me
> if all of the tests in that base class AbstractModelInterpolatorTest
> were still relevant, and someone has to take a look at them. They were
> failing.
>
> There is is still the issue of handling snapshot artifacts,
> but the patch has been tested on a highly concurrent build
> server for some time now, and seems stable for a build
> without any snapshot artifacts.
>
> Regards,
>
> Kristian Rosenvold
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>

Thanks,

Jason

----------------------------------------------------------
Jason van Zyl
Founder,  Apache Maven
http://twitter.com/jvanzyl
----------------------------------------------------------


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org