You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Bryan Pendleton <bp...@gmail.com> on 2015/07/04 19:16:26 UTC
DERBY-6801: Feedback on MessageUtils_2.diff
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
Re: DERBY-6801: Feedback on MessageUtils_2.diff
Posted by Bryan Pendleton <bp...@gmail.com>.
> MessageUtils needs StandardException.getArgumentFerry() to be precise.
>
> How should we solve this ?
Hi Abhinav,
I wonder if we can handle this by introducing a new interface class.
Something like:
interface ExceptionCarrier
{
public static Exception getArgumentFerry( SQLException se );
}
And then:
public class StandardException extends Exception implements ExceptionCarrier
{ ... }
The ExceptionCarrier interface class would be in o.a.d.shared.common.error.ExceptionCarrier.
That way MessageUtils would use the ExceptionCarrier interface, but we would
not need to move the entire Standard Exception class, which is large and
complex and closely tied to the engine internals, to the shared area.
What do you think about this idea?
bryan
Re: DERBY-6801: Feedback on MessageUtils_2.diff
Posted by Bryan Pendleton <bp...@gmail.com>.
Hi Abhinav,
Can you attach your most recent version of MessageUtils patch to DERBY-6801?
I should have some time over the next few days to have a closer look.
bryan
On 7/13/2015 2:00 PM, Abhinav Gupta wrote:
> Hi Bryan,
>
> If I try this alternative way, I get this error
>
> [javac] /home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88: error: ')' expected
> [javac] return (cause instanceof carrier.getClass())
> [javac] ^
> [javac] /home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88: error: illegal start of expression
> [javac] return (cause instanceof carrier.getClass())
> [javac] ^
> [javac] /home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88: error: ';' expected
> [javac] return (cause instanceof carrier.getClass())
> [javac] ^
> [javac] /home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:89: error: not a statement
> [javac] ? cause : null;
> [javac] ^
> [javac] 4 errors
>
>
> I understand from this, that java is unable to parse "carrier.getClass()", am I right ? Also I think I don't understand what would be passed as an argument for the Exception carrier, when getArgumentFerry(SQLException se, Exception carrier) is called ?
>
>
>
> On Mon, Jul 13, 2015 at 8:23 AM, Bryan Pendleton <bpendleton.derby@gmail.com <ma...@gmail.com>> wrote:
>
> Now when I included buildSqlerrmc(), It needs StandardException.getArgumentFerry() to be precise.
>
> How should we solve this ?
>
>
> Hi Abhinav,
>
> Another possibility would be to move StandardException.getArgumentFerry
> out of StandardException and into MessageUtils.
>
> It's not a very complicated function, although it does need to
> be able to run 'instanceof'.
>
> Perhaps a MessageUtils implementation of this function could be
> something like:
>
>
> /**
> * Unpack the exception, looking for a StandardException, which carries
> * the Derby messageID and arguments.
> * @see org.apache.derby.impl.jdbc.SQLExceptionFactory
> * @see org.apache.derby.impl.jdbc.Util
> */
> public static Exception getArgumentFerry(SQLException se, Exception carrier)
> {
> Throwable cause = se.getCause();
> return (cause instanceof carrier.getClass()) ? cause : null;
> }
>
> Something like this might be an alternative approach.
>
> bryan
>
>
Re: DERBY-6801: Feedback on MessageUtils_2.diff
Posted by Abhinav Gupta <ab...@gmail.com>.
Hi Bryan,
If I try this alternative way, I get this error
[javac]
/home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88:
error: ')' expected
[javac] return (cause instanceof carrier.getClass())
[javac]
^
[javac]
/home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88:
error: illegal start of expression
[javac] return (cause instanceof carrier.getClass())
[javac]
^
[javac]
/home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:88:
error: ';' expected
[javac] return (cause instanceof carrier.getClass())
[javac]
^
[javac]
/home/abhinav/Documents/Derby/Derby-6801/java/shared/org/apache/derby/shared/common/error/MessageUtils.java:89:
error: not a statement
[javac] ? cause : null;
[javac] ^
[javac] 4 errors
I understand from this, that java is unable to parse "carrier.getClass()",
am I right ? Also I think I don't understand what would be passed as an
argument for the Exception carrier, when getArgumentFerry(SQLException se,
Exception carrier) is called ?
On Mon, Jul 13, 2015 at 8:23 AM, Bryan Pendleton <bpendleton.derby@gmail.com
> wrote:
> Now when I included buildSqlerrmc(), It needs
>> StandardException.getArgumentFerry() to be precise.
>>
>> How should we solve this ?
>>
>
> Hi Abhinav,
>
> Another possibility would be to move StandardException.getArgumentFerry
> out of StandardException and into MessageUtils.
>
> It's not a very complicated function, although it does need to
> be able to run 'instanceof'.
>
> Perhaps a MessageUtils implementation of this function could be
> something like:
>
>
> /**
> * Unpack the exception, looking for a StandardException, which carries
> * the Derby messageID and arguments.
> * @see org.apache.derby.impl.jdbc.SQLExceptionFactory
> * @see org.apache.derby.impl.jdbc.Util
> */
> public static Exception getArgumentFerry(SQLException se, Exception
> carrier)
> {
> Throwable cause = se.getCause();
> return (cause instanceof carrier.getClass()) ? cause : null;
> }
>
> Something like this might be an alternative approach.
>
> bryan
>
>
Re: DERBY-6801: Feedback on MessageUtils_2.diff
Posted by Bryan Pendleton <bp...@gmail.com>.
> Now when I included buildSqlerrmc(), It needs StandardException.getArgumentFerry() to be precise.
>
> How should we solve this ?
Hi Abhinav,
Another possibility would be to move StandardException.getArgumentFerry
out of StandardException and into MessageUtils.
It's not a very complicated function, although it does need to
be able to run 'instanceof'.
Perhaps a MessageUtils implementation of this function could be
something like:
/**
* Unpack the exception, looking for a StandardException, which carries
* the Derby messageID and arguments.
* @see org.apache.derby.impl.jdbc.SQLExceptionFactory
* @see org.apache.derby.impl.jdbc.Util
*/
public static Exception getArgumentFerry(SQLException se, Exception carrier)
{
Throwable cause = se.getCause();
return (cause instanceof carrier.getClass()) ? cause : null;
}
Something like this might be an alternative approach.
bryan
Re: DERBY-6801: Feedback on MessageUtils_2.diff
Posted by Abhinav Gupta <ab...@gmail.com>.
Hi Bryan,
I was working on 6802, 6803 when I realized this. You had pointed out
earlier that we would be needing DRDAConnThread.buildSqlerrmc() in
MessageUtils.
> 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:
>
But earlier this week when I was removing
MessageUtils.getSQLException(...), I ended up removing the buildSqlerrmc()
that I had refactored in MessageUtils, the reason being, we were weeding
out StandardException that I had moved to shared area for getSQLException.
Now when I included buildSqlerrmc(), it occurred to me that this function
too needs StandardException and all it's supporting classes. It needs
StandardException.getArgumentFerry() to be precise.
How should we solve this ? Should we move StandardException too, to the
shared area again ? I can't seem to get through without it. Meanwhile I'm
attaching MessageUtils, that does not compile at the moment, because of
this.