Hi Pavel,
To your questions...
On 8/8/19 11:07 AM, Pavel Rappo wrote:
...
109: "template method" doesn't describe the method well, the method is private
and not overridable.
update the javadoc.
I can see several questions here. Correct me if I'm wrong. The first one is about the use
of the term "Template Method". This is the name of the behavioral pattern that
has been applied here. We can try to describe what that method does a bit better,
however... here goes the second question. There is no Javadoc generated for classes like
this one. So it's very likely that the documentation we are talking about will be read in
conjunction with the source code in an editor. Combine that with the fact that this is a
test supporting infrastructure and you might see why I think it would be easier to just
outline the intent of the method. People will read the source code anyway. Which is not
to say that documentation can be avoided altogether, it's just that I wouldn't sweat it.
Since its a private method, it is not a template for anything and the
term doesn't apply.
The third question if that method should be overridable. The canonical version
[1] of the Template Method pattern says:
"...Primitive operations that _must_ be overridden are declared pure virtual. The
template method itself should not be overridden..."
Refering the 'Template" pattern doesn't make the documentation clearer.
I don't have a strong opinion about this, only a preference. I prefer that we will make
it overridable only if we find out that the required degree of extensibility cannot be
achieved by introducing additional extension/hook methods (the ones that the template
method calls). Think about it this way. If that method were overridable, then why would
that class be called `BaseLdapServer`? That method is responsible for that class'
"LDAPness"!
Private is fine.
...
178: logger() should be final. The subclass has no reason to override. And it can be
"public final".
final? Yes. public? I'd rather leave it as it is. I can't think of a case where
an *internal* logger should be sticking out of that class. Hence it's not
public. It might be required when subclassing though, hence it's not private or
of default (package) access.
final is enough to avoid hacking around without giving it some thought.
238: isRunning should be synchronized(lock) to make sure each Thread sees the
same value threads that update it in start() and close()
We have already discussed it with you in this thread. The `state` is `volatile`
which is enough for visibility in this query-method.
yes, but there are multiple locking/synchronization constructs that make
the code harder to read
as unnecessary complexity.
...
105: Tests that sleep are prone to intermittent failures on slow or delayed
systems.
True, however, I'm not aware of any general solution of this problem.
General is not needed, and sleeps waste time time and resources.
In this particular test, if there is no possiblity of multiple
connections then CountDownLatch is a good mechanism.
It would be more reliable to countdown *after* the Connection was handled.
As is, it might report success even if handleRequest failed for some reason.
Hm... Let me think. What we are interested in here is whether the connection is
attempted or not. We may do what you are suggesting just to be sure the server
is not failing. That is, the client is served successfully.
The test description is not specific about that.
And it raises questions about the purpose of the environment setup.
Is it all needed. Is handling Unbind in the switch needed (as different
from the default).
<snip>
If there as some suspecion of too many connections
that could be checked after the beforeConnectionHandled called countDown.
Wouldn't that make problems of its own? If I got you right, a time window for
the LDAP client to create more *unwanted* connections could be really small. A
hybrid approach might be even better. We wait for the first connection with a
generous timeout and then give the client some extra time to create more
connections.
It really is fine-tuning. You can't shield completely from arbitrarily slow
systems.
That's fine, a Semaphore can be used to wait for the first connection
and then check with a different timeout for unexpected connections.
Since new infra structure is being introduced, it should be considered to
leverage TestNG
testing facilities and Asserts?
Pardon, where exactly should we use TestNG? Reconnect.java is a plain
(psvm-based) test and BaseLdapServer doesn't know anything about TestNG and
contains no assertions of its own.
TestNG provides a robust set of Assert checks, in many cases they are
more convenient to write and clear in their semantics compared to
separate tests and throws.
It is an opportunity, nothing more.
Roger
Thanks for helping with this!
-Pavel
---------------------------------------------------
[1] Design patterns: elements of reusable object-oriented software (ISBN-13:
978-0201633610)
