Open GoogleCodeExporter opened 9 years ago
(spolier alert: good news below)
I've been struggling since you first suggested this feature between seeing the
usefulness, and being antsy about the "lock in nature" (e.g. whichever of
"start in parallel" or "start in sequence" that we may choose, we'll make some
people discontent), not to mention the funny thing about the different
semantics of GBEMain between projects that use or not multibindings.
So I was pleased with an idea that popped up in my head, and I think satisfies
your needs (which I agree are general) and does not have the drawbacks
aforementioned.
My idea is (all names here are quite tentative, and not thought out, I'd love
better suggestions):
a. leave GBEMain semantics exactly as they are
b. create an "interface GuiceBerryEnvMainlet" with a "void run()" method
c. create a "class ParallelGuiceBerryEnvMainletStarter implements
GuiceBerryEnvMain" that expects multi-bound GBEMainlets (I'm thinking a List
rather than Set)
d. with that, someone is free to either: 1. choose to bind GBEMain to
ParallelGBEMainletStarter, and leverage the multibindings; 2. use their own
GBEMain, as they do now, without honoring the Mainlet multibindings at all; 3.
create their own GBEMainletStarter thingy that honors the Mainlet multibindings
in any way they choose; 4. mix and match all of these
e. we might as well also have a SequentialGBEMainletStarter
Of course, all this can be done even without modifying GuiceBerry proper, but
I'd be more than happy to have these classes/interfaces in GB itself, so as to
encourage the pattern.
Does that satisfy your need, or am I missing something?
Z
Original comment by zorze...@gmail.com
on 13 Feb 2012 at 1:38
That would satisfy my needs perfectly. However I'd like to raise to points and
play devils advocate if I may ;-)
1) I'm worried that adding a the GuiceBerryEnvMainlet infrastructure would
clutter the GuiceBerry Api... there is now the GuiceBerryEnv standard way to
start a server, then this extension (and if you want to get rid of it later it
may be harder)... it could be put into a .contrib or .ext package and/or be an
optional jar.. points worth considering at least. What do you think?
2) Whether GuiceBerryEnvMainlets are multi bound, or if GuiceBerryEnv's
themselves could be multi bound, there is still an awkwardness of the
multibinding api to consider.... having various places in the code base
contribute to the multi binding always feels to loose for an API in my
opinion.... my preference would be to create a dedicated module for this
install(new GuiceBerryEnvMainModule(MyGuiceBerryEnv.class));
or mainlet if you want to go that way?
What do you think? I'm happy to submit a patch either way :-)
Original comment by jongerr...@google.com
on 13 Mar 2012 at 2:24
> 1) I'm worried that adding a the GuiceBerryEnvMainlet infrastructure would
clutter
> the GuiceBerry Api...
Clutter is a legit concern, but the feature is compelling (and the clutter
small), and I'm happy to add the concept as an "advanced" collection of classes
-- just like controllable injections.
> there is now the GuiceBerryEnv standard way to start a server
which would continue existing exactly as today (one reason I like this
approach).
> then this extension (and if you want to get rid of it later it may be
harder)...
extension == useful classes that someone else could have done outside of GB
proper, that I chose to feature, by bundling
harder == not going to happen. I'm ok with that
> it could be put into a .contrib or .ext package and/or be an optional jar..
points worth considering at
> least. What do you think?
Separate java package (no need for a separate jar), just for this stuff plus
#23. How about "components" for a package name? We could name these
GuiceBerryMainComponent and TestWrapperComponent
> there is still an awkwardness of the multibinding api to consider.... having
various places in the
> code base contribute to the multi binding always feels to loose for an API in
my opinion....
> my preference would be to create a dedicated module for this
I don't fully understand how it would look like with a "dedicated module", but
I think all we need is a "GuiceBerryComponets" class with a static method for
adding an entry to the (internally multi-bound) "List<GBMC>" (and another for
the test wrapper thing).
I look forward to patches.
Be warned: 95% of this change will be docs (in the form of javadoc, tutorial
classes, tutorial doc and announcement).
Zorzella
Original comment by zorze...@gmail.com
on 13 Mar 2012 at 5:58
Thanks, this sounds good to me and I think I have enough info to start working
on some patches.
Original comment by jongerr...@google.com
on 13 Mar 2012 at 6:02
Hi,
Included is a patch which contains TestWrapperModule, installing this and
calling the associated static method allows for installing multiple
TestWrappers. Included a test.
Issue #23 is very similar, so I will await your feedback on this to make sure
it is in the general direction you're happy with before making a start.
Original comment by jongerr...@google.com
on 13 Mar 2012 at 11:43
Attachments:
My apologies for taking this long to review this.
There are a few things I like and a few I dislike about the patch:
* if I get this right, you decided to, instead of creating a new
TestWrapperlet, or TestWrapperComponent or whatever, leverage a binding
annotation. I must confess that at first look I disliked the idea, but the more
I think about it, the more I like it. So you leverage a binding annotation, so
as to (for example) allow you to choose to use a TestWrapper someone else wrote
(originally as a full wrapper), and instead multibind it. It has the added
benefit of not creating a new interface that looks exactly like TestWrapper.
The only downside I can see is that it makes the intent of TestWrapper more
complicated. I need to sleep over this, but I'm reasonably convinced I the pros
win handily over the cons. Your patch will need, though, to include changes
(additions) to the TestWrapper javadoc.
* We want to use List rather than Set, so the iteration order is
predictable/stable.
* I don't like the name CompositeTestWrapper. Something like
SequentialTestWrapperExecutor feels right, particularly because...
* ... we should provide for a ParallelTestWrapperExecutor counterpart right off
the bet
* Both of these *Executors should be public
* I don't think that TestWrapperModule is useful. Other than the static method
(see below), it only adds a binding that IMHO should just be done in people's
GB modules.
* I think the static multibind method should be in, say, a GuiceBerryComponents
class with just static methods. I think it even makes sense that the same class
have both multibind methods for TWrapper and GBEMain
* @Internal should be public -- so as, e.g., to allow for someone to write
their own MyNotQuiteParallelTestWrapperExecutor, if they so wish, and simply
replace one executor for another. Not that I think this will be done, but I
feel the walled garden alternative here is not really beneficial in a
significant way.
* @Internal should be named something like @Multibound (and I think we should
use the same binding annotation for both TestWrapper and GBEMain)
* I'm not familiar with using "@Qualifier @Documented @Retention(RUNTIME)" to
create a binding annotation. It's not how it's done elsewhere in GuiceBerry nor
I see anything in BindingAnnotation's javadoc about there being a different way
of doing it. Do you have a particular reason to use this construct? Any link
with info about this?
* this needs lots of docs: particularly javadoc for all the public classes and
methods, and a a new tutorial for this.
BTW, despite the nitpicking, I really like the idea and I'm happy with what I
think this will look like!!!
Original comment by zorze...@gmail.com
on 24 Mar 2012 at 7:27
Find attached a new patch, and I've made a slight change in direction in
response to your comments, its still pretty rough but if you are happy with the
direction I can start to add some polish to the patch :-) I'll address you
comments below:-
* Guice Multibindings don't support List, BUT the Set is iterable in the order
the components were bound, although its recommended not to rely on ordering as
you can be relying on the order of module installation, you can read more about
it here:-
http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject/mu
ltibindings/Multibinder.html
* I changed the name from CompositeTestWrapper to SequentialTestWrapperExecutor
as requested and also added a ParallelTestWrapperExecutor (they share all the
same code with the variation being the ExecutorService implementation used. If
you don't like having the two sub classes with different constructor
configurations an alternative is to remove them, but require the user to bind
an ExecutorService manually. Perhaps there could be an @GuiceBerryComponents
annotation so it doesn't clash with other ExecutorServices bound.... personally
I prefer the subclasses though.
* I made both Executors public
* I added a similar implementation for *GuiceBerryEnvMainExecutor
* I removed the @Internal annotation, its purpose was to be package private for
the purpose of the multibindings, in order to create that walled garden
implementation, so all bindings are hidden by the static method, and would not
clash with other bindings of TestWrapper/GuiceBerryEnvMain elsewhere in the
system. I think you are probably right though, the walled garden approach is a
waste of time, since these interfaces are very specific to GuiceBerry and any
bindings of them will be in the context of GuiceBerry.
* I removed the TestWrapperModule. I removed the static method too, I'm not
sure how helpful it is now.
* I added a new tutorial class, I hope its in the right location, it feels more
like an integration test though, but if you have any guidance on this I'd
appreciate it... should I add some text for the tutorial, is this just on
Google Docs?
If you are happy with the direction, let me know and I'll go through and add
more docs etc.
Original comment by jongerr...@google.com
on 25 Mar 2012 at 6:23
Attachments:
I'm very happy with the process:
*********************************
* Guice Multibindings don't support List, BUT the Set is iterable in the order
the components were bound
Ok
* I changed the name from CompositeTestWrapper to SequentialTestWrapperExecutor
as requested and also added a ParallelTestWrapperExecutor
excellent. ParallelGuiceBerryEnvMain lacks the "Executor" suffix.
> (they share all the same code with the variation being the ExecutorService
I love it -- this is excellent! Can't you make them delegate rather than
subclass, so as to make the "base class" final? Also, I don't quite understand
(and it's not javadoc'ed) the semantics of what happens when one or more
exceptions are thrown from the multibound TWs/GBEMs.
* I made both Executors public
Ok.
* I added a similar implementation for *GuiceBerryEnvMainExecutor
Ok.
* I removed the @Internal annotation, its purpose was to be package private for
the purpose of the multibindings
I told you I'd sleep on using the annotated TWrapper/GBEMain instead of
creating new classes. I'm fine with that. Note, though, that this will likely
be a little less of a panacea than I first anticipated, since it's much more
common than not to have TWs/GBEMs be @Provides methods, which are not
immediately transformable into multibindings. Nevertheless, I'm still fine with
the usage.
I do have to sleep over the idea of not using an annotation, which is a minor
point. I'll let you know.
* I removed the TestWrapperModule. I removed the static method too, I'm not
sure how helpful it is now.
The static method still seems useful, particularly wrt to documentation and
usage findability. I was also thinking it can also make syntax terser with
varargs, i.e. a signature like:
static void multiBindTestWrapper(Binder, TestWrapper...)
* I added a new tutorial class, I hope its in the right location, it feels more
like an integration test though, but if you have any guidance on this I'd
appreciate it... should I add some text for the tutorial, is this just on
Google Docs?
Yes, the tutorials end up being the simplest integration tests of the features
(clearest e.g. controllable injections). It should be in the
'junit4/tutorial_0_basic' tree (each tutorial is given in the 4 testing tools
supported -- we'll leave this for last; it's a bit tedious, but not rather
simple to do). Also put a .txt as part of the patch that I'll not submit, but
rather I'll include in the Google Docs tutorial doc.
> If you are happy with the direction, let me know and I'll go through and add
more docs etc.
Very happy. We only need more doc and some polish.
Oh, but one last thing: I'd rather not add a dep to mockito (or anything else),
unless there's a pretty good reason...
Original comment by zorze...@gmail.com
on 26 Mar 2012 at 4:58
* ParallelGuiceBerryEnvMain lacks the "Executor" suffix.
-- Done
* Can't you make them delegate rather than subclass, so as to make the "base
class" final?
-- I could, but I think this would cause some other awkwardness in the code,
namely that:-
* If we delegate we'd have to either inject the TestWrapperExecutor or just
'new' it. If we inject it then we'd need to bind ExecutorService somewhere and
with an annotation (because I don't think we should bind a global
ExecutorService since its probably a very common binding that may clash), and
then we'd need to control which executor service gets bound and there could be
four possible scenarios Parallel/Sequential Vs TestWrapper/GuiceBerryEnvMain.
'new'ing it feels awkward too, we'd need to create the delegate within each
concrete class, save it as a member and override the run method which is more
boiler plate. What would you like to achieve by making
{GuiceBerryEnvMain,TestWrapper} final? Are these classes you'd like to expose
in the API? If not, we can make the classes package private, then only the
XXSequentialExecutor and XXParallelExecutor are part of the public API, then
the implementation details (extension vs composition matter less). Another
option which I like is to have a GuiceBerryComponents class of static methods
which could install the bindings on the users behalf e.g:
public class GuiceBerryComponents {
private GuiceBerryComponents() { // Utility methods, non-instantiable }
public Module newParallelGuiceBerryMainModule() {
return new AbstractModule() {
protected void configure() }
bind(GuiceBerryEnvMain.class).to(GuiceBerryEnvMainExecutor.class);
bind(ExecutorService.class).annotatedWith(GuiceBerryEnvMainExecutor.class).toInstance(Executors.newCachedThreadPool());
}
}
}
}
There would be similar methods for Sequential and corresponding ones for
TestWrapperExecutor too. The advantage of this approach is that it would remove
the four boiler plate concrete classes. It might be possible to replace the
custom annotation (or @Named) for the executor service with PrivateModules and
expose() but I'd need to check if that works with the multi bindings.
GuiceBerryComponents would also be the class to have the helper methods for
adding the multi bindings.
Yet another approach would be to go back to the Walled Garden idea of having a
module that takes care of the internal bindings above but also exposes an API
to support the multibindings.
The extension of package private base classes is the option I've taken in this
patch as I'd rather not change too much unless I'm sure of the direction.
* I don't quite understand (and it's not javadoc'ed) the semantics of what
happens when one or more exceptions are thrown from the multibound TWs/GBEMs.
I added javadoc for this onto the GuiceBerryEnvMainExecutor and
TestWrapperExecutor… basically this follows the semantics of the
ExecutorService, which when we call Future.get() to retrieve the void results,
we'd be notified of an ExecutionException, and we'd end up propagating the
first one. The alternative would be to catch and hold on to each
ExecutionException and then throw some kind of UmbrellaException which contains
all Exceptions that were caught.
* The static method still seems useful, particularly wrt to documentation and
usage findability. I was also thinking it can also make syntax terser with
varargs, i.e. a signature like:
static void multiBindTestWrapper(Binder, TestWrapper...)
We could go this way, but we'd end up copying a lot of the syntax of
LinkedBindingBuilder, such as to(), toInstance(), toProvider() which we
probably want to avoid.
Another approach would be to have this self documenting done using an abstract
module, with the disadvantages that users need to subclass, but the advantages
of a better syntax (callers are not passing binder() to the static methods).
public class GuiceBerryComponentsModule extends AbstractModule {
protected LinkedBindingBuilder<GuiceBerryEnvMain> multiBindGuiceBerryEnvMain() {
Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder(), GuiceBerryEnvMain.class);
return envMainBinder.addBinding();
}
}
client classes could then be written like:-
public class MyGuiceBerryComponentsModule extends GuiceBerryComponentsModule {
protected void configure() {
bind(TestWrapper.class).to(SequentialTestWrapper.class);
multiBindTestWrapper().to(FirstTestWrapper.class);
multiBindTestWrapper().toProvider(TestWrapperProvider.class);
bind(GuiceBerryEnvMain.class).to(ParallelGuiceBerryEnvMain.class);
mutliBindGuiceBerryEnvMain().toInstance(new MyEnvMain());
}
}
* the tutorials end up being the simplest integration tests of the features
(clearest e.g. controllable injections). It should be in the
'junit4/tutorial_0_basic' tree
-- Done.
I'll get back to work on this when I receive your advice on some of the above
questions, sorry that its taken so long to get around to it.
Original comment by jonat...@indiekid.org
on 10 Apr 2012 at 5:16
Attachments:
Apologies on my end, I've been very busy.
1. lest we forget, this adds a dep to guice-multibindings-3.0.jar. Please help
me remember to add this in the setup docs
2. What would you like to achieve by making {GuiceBerryEnvMain,TestWrapper}
final?
The little syntax boilerplate saved by using inheritance for the sole purpose
of overriding the constructor does not warrant, imho, the abuse of inheritance.
I patched your CL, and modified the sequential to see what it looks like, and I
like what I see, presented at the bottom of this message (from package
declaration to the end, though the javadoc might need tweaks). BTW, before you
have ideas to try to convince me that the class extending thingy is a good
idea, you should chat with Marty, he'll tell you I'm a hopeless "class Foo
extends Bar" hater :) And last I let myself be talked into it (that'd be
GuiceBerryModule), I regretted it a bunch, as you know...
3. The new classes lack a copyright at the top
4. I don't like a GuiceBerryComponentsModule for multibindings (see "extends
hater" bit above). At the least, it feels strictly better to change this (found
in the test class):
Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder(), GuiceBerryEnvMain.class);
envMainBinder.addBinding().to(IncrementingGuiceBerryEnvMain.class);
to:
Multibinder<GuiceBerryEnvMain> envMainBinder = GuiceBerryEnvMainMultibinder.multibinder(binder());
envMainBinder.addBinding().to(IncrementingGuiceBerryEnvMain.class);
and it feels strictly better to go further and do:
GuiceBerryEnvMainMultibinder.addBinding(binder()).to(IncrementingGuiceBerryEnvMain.class);
where "addBinding" here simply returns a
LinkedBindingBuilder<GuiceBerryEnvMain>, i.e. the impl would look like this:
public static LinkedBindingBuilder<GuiceBerryEnvMain> addBinding(Binder binder) {
Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder, GuiceBerryEnvMain.class);
return envMainBinder.addBinding();
}
No duplication of code, nothing. Just a simpler, documentable, findable way to
multibind GBEMains... I tried it and the tests still pass. What am I missing?
Thanks for the good work,
Z
**********************
package com.google.guiceberry.components;
import com.google.guiceberry.GuiceBerryEnvMain;
import javax.inject.Inject;
import java.util.Set;
import java.util.concurrent.Executors;
/**
* A Guiceberry {@link com.google.guiceberry.GuiceBerryEnvMain} that is used internally to support the
* execution of Multi-bound GuiceBerryEnvMains.
*
* <p>This class executes each bound env main sequentially in the order they were bound.
*
* @author jongerrish@google.com (Jonathan Gerrish)
*/
public class SequentialGuiceBerryEnvMainExecutor implements GuiceBerryEnvMain {
private final GuiceBerryEnvMainExecutor delegate;
@Inject
public SequentialGuiceBerryEnvMainExecutor(Set<GuiceBerryEnvMain> envMains) {
delegate = new GuiceBerryEnvMainExecutor(envMains, Executors.newSingleThreadExecutor());
}
public void run() {
delegate.run();
}
}
Original comment by zorze...@gmail.com
on 18 Apr 2012 at 12:04
Original issue reported on code.google.com by
jongerr...@google.com
on 8 Feb 2012 at 8:18