db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Knut Anders Hatlen <Knut.Hat...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-733) Starvation in RAFContainer.readPage()
Date Thu, 15 Dec 2005 10:31:11 GMT
Mike Matrigali <mikem_app@sbcglobal.net> writes:

> I have reviewed and committed the following patch.  I have the following
> comments which weren't enough to hold up this patch:
>
> 1) Will you at some point be contributing the performance tests which
>     you used to verify this fix, and the original test which showed the
>     problem?  I know this is a hard problem, just wondering if you are
>     working on solving it.

I don't think I can contribute the test that found the problem, as it
is part of a larger test framework. I can however make a small test
that reproduces the problem and attach it to the JIRA issue.

> 2) minor nit - could you try to keep lines under 80 chars.

Ok, sorry about that.

> 3) I was a little surprised that catch blocks did not do anything
>     on a released system.  Then thinking about it, it seemed that ok
>     since the added code is not actually using the lock mechanism to
>     protect anything, but instead just to schedule the enclosed work
>     more fairly.  In fact in jdk1.4 systems the code has to work
>     without the lock calls at all.  I have not idea what kind of
>     exceptions might happen.

The checked exceptions that Method.invoke() might throw are
IllegalAccessException, IllegalArgumentException and
InvocationTargetException. lock() does not throw exceptions, but
unlock() throws IllegalMonitorStateException if the current thread is
not the owner of the lock. My reasoning was that this could only
happen if someone put a bogus ReentrantLock class in their classpath,
which is highly unlikely, and in that case we could just fall back to
pre-1.5 behaviour.

>     The only case that really concerned me
>     is if lock worked but unlock failed - you may want to think about
>     that case and see if you should raise an exception.

The only case in which unlock() fails, is when lock() has failed or
hasn't been invoked. We can easily address this issue by adding
hasJava5FairLocks = false to the catch blocks and fall back to the old
behaviour.

>     May be useful to add comments why no exception handling is necessary.

I will do that.

Thanks for reviewing and committing,

Knut Anders


Mime
View raw message