Hi Abhinav,
I spent some time thinking about MessageUtils_2.diff.
I'd like to see us refine this still further, because
I think we want to avoid moving many of these other
classes into the shared area wholesale.
In most of these cases, I think we can refine things
and make a smaller and more precise patch, even though
that will require some more modifications to the MessageUtils class.
For example, let's consider AppRequester.
The only reason that we needed AppRequester in
MessageUtils is for this line:
int maxlen = (sqlerrmc == null) ? -1 : Math.min(sqlerrmc.length(),
appRequester.supportedMessageParamLength());
The supportedMessageParamLength() method is pretty trivial:
public static final int DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH = 2400;
protected int supportedMessageParamLength() {
return DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH;
}
So let's not bring all of AppRequester over to the shared
library just for this one bit.
Instead, FOR NOW, let's just duplicate these 4 lines of code
into MessageUtils.java.
I realize that seems bad, but that particular named
constant is already a bit messed up in the code:
$ find . -name '*.java' -exec grep DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH {} \;
-print
return Limits.DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH;
./java/drda/org/apache/derby/impl/drda/AppRequester.java
public static final int DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH = 2400;
./java/engine/org/apache/derby/iapi/reference/Limits.java
Types.VARCHAR,
Limits.DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH),
./java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java
public static final int DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH = 2400;
return DB2_JCC_MAX_EXCEPTION_PARAM_LENGTH;
./java/shared/org/apache/derby/shared/common/error/AppRequester.java
So I'll file a new subtask of DERBY-6773 to clean up this
little duplication of code, which we can do as a follow-up
after this step is complete.
Regarding the other classes, it seems that nearly all of them
are present in order to provide support for the method
MessageUtils.getSQLException(...)
Can we simply remove getSQLException() from MessageUtils?
It doesn't seem to me that we should need it, and if we remove
getSQLException() from MessageUtils, and also remove the
helper method wrapArgsForTransportAcrossDRDA,
then I believe we can remove all the classes:
- MessageService
- ExceptionFactory
- ArrayUtil
- BundleFinder
- StandardException
Next, for SQLState.java, I think that maybe is just a typo. The
correct SQLState is already in the shared library, in
org.apache.derby.shared.common.reference.SQLState
So I think you can remove the SQLState.java from your patch,
and simply add to MessageUtils.java:
import org.apache.derby.shared.common.reference.SQLState;
That leaves us with ShutdownException.
For ShutdownException, the only reason we had to bring it
into this patch is because the getLocalizedMessage()
method wants to catch and ignore it.
That is interesting; let me think about that one for a while.
It's possible that moving ShutdownException into the shared
library is the best way to deal with this, but there may be
other ways.
So, to summarize, this is my proposal:
1) Duplicate 4 lines of code from AppRequester into MessageUtils,
and file a follow-on sub-task of DERBY-2773 to remove this
duplication (by refactoring AppRequester, Limits, and
DataDictionaryImpl to use the new MessageUtils method),
after MessageUtils has been committed.
2) Remove the getSQLException() methods and change
MessageUtils so it doesn't subclass ExceptionFactory.
3) Remove the classes: AppRequester, ExceptionFactory,
ArrayUtil, BundleFinder, MessageService, and
StandardException
4) We still have to do more study on ShutdownException
Does that make sense?
thanks,
bryan
