i'm a new user of grizzly, embedded for jaxws. here are my findings
after the first steps.
i was not able to achieve my goals without downloading the grizzly
source code. in other libraries like spring i rarely have to dig into
the code to understand what's going on.
- the javadoc alone is, in the areas where i needed it, too sparse.
- the method names and parameters alone were not clear enough to me
- there were some surprises
here are some examples:
-----------------------------------------------------------------------
----------------
ThreadPoolConfig config = ThreadPoolConfig.defaultConfig();
ok so now i've got the default config. there is a method .copy() on it.
should i call it?
1) there is no javadoc on the method.
2) the method name suggests that i get the default config and should
make a copy unless i really want to modify the defaults. looking at the
code revealed that my intuition was wrong.
suggestion:
1) add javadoc
2) maybe rename method (set to deprecated and add new one):
copyDefaultConfig(). at least the method isn't called
getDefaultConfig(). but i usually like the verbNoun() concept.
-----------------------------------------------------------------------
----------------
ThreadPoolConfig is a builder
it took me a moment to figure out that this class is the configuration
itself as well as a builder. the class header currently has zero
javadoc, i would mention this.
-----------------------------------------------------------------------
----------------
networkListener.getTransport().setWorkerThreadPoolConfig(config);
apidoc says:
Set the {_at_link ThreadPoolConfig} to be used by the worker thread
pool.
@param workerConfig worker thread pool configuration.
looking at the code reveals:
if (isStopped()) {
this.workerPoolConfig = workerPoolConfig;
}
1) the javadoc contains a lot of noise. when the setter is called
setWorkerThreadPoolConfig() then the javadoc "@param workerConfig
worker thread pool configuration" is obvious.
2) the javadoc doesn't say that the call is silently ignored when the
server is started already.
3) i disagree with the behavior of ignoring a setter when the server is
started, and would instead expect and document an
IllegalStateException.
-----------------------------------------------------------------------
----------------
HttpServer: incomplete javadoc
the class header itself has zero javadoc.
the methods start and stop only say the obvious, but don't say that
they silently ignore the call if it is in that state already.
also, i'd be interested in how stop() behaves; does it at first disable
new incoming requests, and let currently running ones finish
gracefully? or is it an immediate stop and kills them? are there
alternatives to this call?
-----------------------------------------------------------------------
----------------
ThreadPoolConfig again: useless javadoc
many methods look like this one:
/**
* @return the queuelimit
*/
public int getQueueLimit() {
return queueLimit;
}
i would prefer to have the noise "@return the queuelimit" removed so
that my eyes have less to process.
this one should indeed have some documentation imo. the special value
-1 is used, and not in one place is documented what it means. i was
able to figure it out, but mentioning it would remove all doubt and
also remind new developers of the special case when they make changes
to the code.
-----------------------------------------------------------------------
----------------
null handling
the current concept of null handling is to not check for it. as a
result exceptions occur late, and the cause is harder to find.
also, the user doesn't know if he's allowed to hand in null at all
(maybe the concept is to never allow null?).
i personally would prefer to have methods annotated using jetbrains'
null annotations @Nullable and NotNull. they document
what's allowed plus enforce it, throwing an IllegalArgumentException
instantly instead of getting a NPE anywhere later on.
same for getters where atm i have to guess if the returned value could,
under some circumstances, be null. without the
annotation/documentation i'm forced to check for it.
end of rant, and thanks for the software. maybe one thing or two of
what i believe would improve the api are considered.