db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dyre.Tjeldv...@Sun.COM
Subject Re: [jira] Commented: (DERBY-938) ContextManager needs to be optimized
Date Mon, 13 Feb 2006 16:18:41 GMT
>>>>> "DJD" == Daniel John Debrunner <djd@apache.org> writes:

    DJD> Dyre.Tjeldvoll@Sun.COM wrote:
    >> Provide specalized accessor method to get the most recently pushed ExecutionContext
and StatementContext.
    >> This lets clients of the ContextManager class access these Contexts
    DJD> directly without going through the HashMap.

    DJD> This does break the intended model of the ContextManager in that it is
    DJD> meant to be independent of other code in the system, like most of the
    DJD> services. Thus, in theory, a store system could be built without
    DJD> dragging in any language code, now such a system would drag in language
    DJD> code.

Thank you for looking at the patch and providing valuable and helpful
comments. You are absolutely right, of course, the ContextManager
should not have to pull in language code. 

This added dependency is ALMOST unnecessary (for the patch). Firstly,
the two added import statements can be removed if
getStatementContext() and getExecutionContext() are modified to return
a Context reference, (all the clients must then perform the casts
themselves, just like with getContext(String)).

Then we're only left with the equality-tests in pushContext(Context). The first
of these, which tests if the pushed Context is a StatementContext,
does not introduce a new dependency, as far as I can tell.
ContextManager.java already imports 
org.apache.derby.iapi.reference.Property
and the LANG_STATEMENT is located in 
org.apache.derby.iapi.reference.ContextId.

Then we're only left with that unfortunate dependency on 
org.apache.derby.iapi.sql.execute.ExecutionContext.CONTEXT_ID. Now, if
it would be OK to move this into
org.apache.derby.iapi.reference.ContextId.
as well, the problem would be solved.

So that brings up the question of why 
org.apache.derby.iapi.reference.ContextId 
currently has only two entries:

package org.apache.derby.iapi.reference;

public interface ContextId {


        String LANG_CONNECTION = "LanguageConnectionContext";
        String LANG_STATEMENT = "StatementContext";

}

While all the other 15 or so Context implementations define their
ContextId internally, e.g.:

org.apache.derby.iapi.sql.execute.ExecutionContext.java:

public interface ExecutionContext extends Context {

        /**
         * this is the ID we expect execution contexts
         * to be stored into a context manager under.
         */
        String CONTEXT_ID = "ExecutionContext";


Finally I would like to say that I can understand why people would
object to the propsed solution even if it is possible to remove the
dependency problem. I too think that having special methods for two
types of Contexts is ugly and clutters up the interface of
ContextManager, and I considered a couple of alternatives. 

The first was to re-write the whole ContextManager. I abandoned that
right away because I don't know the code well enough, and because the
patch would be huge. The second approach was to change the ContextId
from a String to an int. I abandoned that idea because it too would
have yielded a huge diff, and it would have made it more difficult to add new
Contexts (need to maintain all ids in a central locaton to make sure
the same number isn't used twice). While an array lookup probably is
faster than a hash table lookup one would still be doing unnecessary
work, and I found code where the ContextId is used outside the
ContextManager, and this code would have to change if the ContextIds aren't
Strings. 

So I'm not "attached" to this particular solution in any way, and if
anyone can propose a better solution to this problem I would love to
substitute that in the patch, and or omit this part altogether, if the
solution should be submitted as a separate patch (or part of another).

Sorry about the long email.

-- 
dt


Mime
View raw message